[pacman-dev] [PATCH 2/4] Change sprintf function using for snprintf
* Size examined str* function usage is a common coding practice, because it's more safer to avoid breakage while using str* functions. Signed-off-by: Laszlo Papp <djszapi@archlinux.us> --- lib/libalpm/add.c | 4 ++-- lib/libalpm/be_files.c | 10 +++++----- lib/libalpm/conflict.c | 2 +- lib/libalpm/db.c | 4 ++-- lib/libalpm/trans.c | 2 +- lib/libalpm/util.c | 16 +++++++++------- src/pacman/sync.c | 2 +- src/pacman/util.c | 2 +- 8 files changed, 22 insertions(+), 20 deletions(-) diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index ebcd6a5..209c38e 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -311,7 +311,7 @@ static int extract_single_file(struct archive *archive, size_t backup_len = strlen(oldbackup) + 34; MALLOC(backup, backup_len, RET_ERR(PM_ERR_MEMORY, -1)); - sprintf(backup, "%s\t%s", oldbackup, hash_pkg); + snprintf(backup, backup_len, "%s\t%s", oldbackup, hash_pkg); backup[backup_len-1] = '\0'; FREE(oldbackup); backups->data = backup; @@ -462,7 +462,7 @@ static int extract_single_file(struct archive *archive, hash = alpm_compute_md5sum(filename); MALLOC(backup, backup_len, RET_ERR(PM_ERR_MEMORY, -1)); - sprintf(backup, "%s\t%s", oldbackup, hash); + snprintf(backup, backup_len, "%s\t%s", oldbackup, hash); backup[backup_len-1] = '\0'; FREE(hash); FREE(oldbackup); diff --git a/lib/libalpm/be_files.c b/lib/libalpm/be_files.c index ffbaa8d..36a9cfc 100644 --- a/lib/libalpm/be_files.c +++ b/lib/libalpm/be_files.c @@ -67,7 +67,7 @@ static time_t getlastupdate(pmdb_t *db) dbpath = _alpm_db_path(db); /* dbpath + '.lastupdate' + NULL */ MALLOC(file, strlen(dbpath) + 12, RET_ERR(PM_ERR_MEMORY, ret)); - sprintf(file, "%s.lastupdate", dbpath); + snprintf(file, strlen(dbpath), "%s.lastupdate", dbpath); /* get the last update time, if it's there */ if((fp = fopen(file, "r")) == NULL) { @@ -103,7 +103,7 @@ static int setlastupdate(pmdb_t *db, time_t time) dbpath = _alpm_db_path(db); /* dbpath + '.lastupdate' + NULL */ MALLOC(file, strlen(dbpath) + 12, RET_ERR(PM_ERR_MEMORY, ret)); - sprintf(file, "%s.lastupdate", dbpath); + snprintf(file, strlen(dbpath), "%s.lastupdate", dbpath); if((fp = fopen(file, "w")) == NULL) { free(file); @@ -211,7 +211,7 @@ int SYMEXPORT alpm_db_update(int force, pmdb_t *db) len = strlen(db->treename) + strlen(DBEXT) + 1; MALLOC(dbfile, len, RET_ERR(PM_ERR_MEMORY, -1)); - sprintf(dbfile, "%s" DBEXT, db->treename); + snprintf(dbfile, len, "%s" DBEXT, db->treename); dbpath = alpm_option_get_dbpath(); @@ -241,7 +241,7 @@ int SYMEXPORT alpm_db_update(int force, pmdb_t *db) /* form the path to the db location */ len = strlen(dbpath) + strlen(db->treename) + strlen(DBEXT) + 1; MALLOC(dbfilepath, len, RET_ERR(PM_ERR_MEMORY, -1)); - sprintf(dbfilepath, "%s%s" DBEXT, dbpath, db->treename); + snprintf(dbfilepath, len, "%s%s" DBEXT, dbpath, db->treename); /* uncompress the sync database */ checkdbdir(db); @@ -378,7 +378,7 @@ static char *get_pkgpath(pmdb_t *db, pmpkg_t *info) dbpath = _alpm_db_path(db); len = strlen(dbpath) + strlen(info->name) + strlen(info->version) + 3; MALLOC(pkgpath, len, RET_ERR(PM_ERR_MEMORY, NULL)); - sprintf(pkgpath, "%s%s-%s/", dbpath, info->name, info->version); + snprintf(pkgpath, len, "%s%s-%s/", dbpath, info->name, info->version); return(pkgpath); } diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c index e934c01..e27649c 100644 --- a/lib/libalpm/conflict.c +++ b/lib/libalpm/conflict.c @@ -525,7 +525,7 @@ alpm_list_t *_alpm_db_find_fileconflicts(pmdb_t *db, pmtrans_t *trans, /* check if all files of the dir belong to the installed pkg */ if(!resolved_conflict && S_ISDIR(lsbuf.st_mode) && dbpkg) { char *dir = malloc(strlen(filestr) + 2); - sprintf(dir, "%s/", filestr); + snprintf(dir, strlen(filestr)+2, "%s/", filestr); if(alpm_list_find_str(alpm_pkg_get_files(dbpkg),dir)) { _alpm_log(PM_LOG_DEBUG, "check if all files in %s belongs to %s\n", dir, dbpkg->name); diff --git a/lib/libalpm/db.c b/lib/libalpm/db.c index dca5452..a7e6d18 100644 --- a/lib/libalpm/db.c +++ b/lib/libalpm/db.c @@ -369,12 +369,12 @@ const char *_alpm_db_path(pmdb_t *db) if(db->is_local) { pathsize = strlen(dbpath) + strlen(db->treename) + 2; CALLOC(db->_path, 1, pathsize, RET_ERR(PM_ERR_MEMORY, NULL)); - sprintf(db->_path, "%s%s/", dbpath, db->treename); + snprintf(db->_path, pathsize, "%s%s/", dbpath, db->treename); } else { pathsize = strlen(dbpath) + 5 + strlen(db->treename) + 2; CALLOC(db->_path, 1, pathsize, RET_ERR(PM_ERR_MEMORY, NULL)); /* all sync DBs now reside in the sync/ subdir of the dbpath */ - sprintf(db->_path, "%ssync/%s/", dbpath, db->treename); + snprintf(db->_path, pathsize, "%ssync/%s/", dbpath, db->treename); } _alpm_log(PM_LOG_DEBUG, "database path for tree %s set to %s\n", db->treename, db->_path); diff --git a/lib/libalpm/trans.c b/lib/libalpm/trans.c index aea71db..f4a6879 100644 --- a/lib/libalpm/trans.c +++ b/lib/libalpm/trans.c @@ -113,7 +113,7 @@ static alpm_list_t *check_arch(alpm_list_t *pkgs) const char *pkgver = alpm_pkg_get_version(pkg); size_t len = strlen(pkgname) + strlen(pkgver) + strlen(pkgarch) + 3; MALLOC(string, len, RET_ERR(PM_ERR_MEMORY, invalid)); - sprintf(string, "%s-%s-%s", pkgname, pkgver, pkgarch); + snprintf(string, len, "%s-%s-%s", pkgname, pkgver, pkgarch); invalid = alpm_list_add(invalid, string); } } diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index d910809..553591d 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -315,7 +315,7 @@ int _alpm_unpack(const char *archive, const char *prefix, alpm_list_t *list, int st = archive_entry_stat(entry); entryname = archive_entry_pathname(entry); - + if(S_ISREG(st->st_mode)) { archive_entry_set_perm(entry, 0644); } else if(S_ISDIR(st->st_mode)) { @@ -394,7 +394,7 @@ int _alpm_rmrf(const char *path) } for(dp = readdir(dirp); dp != NULL; dp = readdir(dirp)) { if(dp->d_ino) { - sprintf(name, "%s/%s", path, dp->d_name); + snprintf(name, PATH_MAX, "%s/%s", path, dp->d_name); if(strcmp(dp->d_name, "..") && strcmp(dp->d_name, ".")) { errflag += _alpm_rmrf(name); } @@ -651,7 +651,9 @@ int _alpm_lstat(const char *path, struct stat *buf) char SYMEXPORT *alpm_compute_md5sum(const char *filename) { unsigned char output[16]; + int soutput = sizeof(output); char *md5sum; + int smd5sum = 33; int ret, i; ALPM_LOG_FUNC; @@ -659,7 +661,7 @@ char SYMEXPORT *alpm_compute_md5sum(const char *filename) ASSERT(filename != NULL, return(NULL)); /* allocate 32 chars plus 1 for null */ - md5sum = calloc(33, sizeof(char)); + md5sum = calloc(smd5sum, sizeof(char)); ret = md5_file(filename, output); if (ret > 0) { @@ -667,11 +669,11 @@ char SYMEXPORT *alpm_compute_md5sum(const char *filename) } /* Convert the result to something readable */ - for (i = 0; i < 16; i++) { - /* sprintf is acceptable here because we know our output */ - sprintf(md5sum +(i * 2), "%02x", output[i]); + for (i = 0; i < soutput; i++) { + /* snprintf is acceptable here because we know our output */ + snprintf(md5sum +(i * 2), smd5sum-i*2, "%02x", output[i]); } - md5sum[32] = '\0'; + md5sum[smd5sum-1] = '\0'; _alpm_log(PM_LOG_DEBUG, "md5(%s) = %s\n", filename, md5sum); return(md5sum); diff --git a/src/pacman/sync.c b/src/pacman/sync.c index a2ef616..1507dd5 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -117,7 +117,7 @@ static int sync_cleandb_all(void) { * and only the unused sync dbs in dbpath/sync/ */ ret += sync_cleandb(dbpath, 0); - sprintf(newdbpath, "%s%s", dbpath, "sync/"); + snprintf(newdbpath, PATH_MAX, "%s%s", dbpath, "sync/"); ret += sync_cleandb(newdbpath, 1); printf(_("Database directory cleaned up\n")); diff --git a/src/pacman/util.c b/src/pacman/util.c index 1143bef..f4e1756 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -128,7 +128,7 @@ int rmrf(const char *path) for(dp = readdir(dirp); dp != NULL; dp = readdir(dirp)) { if(dp->d_ino) { char name[PATH_MAX]; - sprintf(name, "%s/%s", path, dp->d_name); + snprintf(name, PATH_MAX, "%s/%s", path, dp->d_name); if(strcmp(dp->d_name, "..") && strcmp(dp->d_name, ".")) { errflag += rmrf(name); } -- 1.6.4.4
On Sun, Oct 18, 2009 at 6:03 AM, Laszlo Papp <djszapi2@gmail.com> wrote:
* Size examined str* function usage is a common coding practice, because it's more safer to avoid breakage while using str* functions.
Signed-off-by: Laszlo Papp <djszapi@archlinux.us> --- lib/libalpm/add.c | 4 ++-- lib/libalpm/be_files.c | 10 +++++----- lib/libalpm/conflict.c | 2 +- lib/libalpm/db.c | 4 ++-- lib/libalpm/trans.c | 2 +- lib/libalpm/util.c | 16 +++++++++------- src/pacman/sync.c | 2 +- src/pacman/util.c | 2 +- 8 files changed, 22 insertions(+), 20 deletions(-)
diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index ebcd6a5..209c38e 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -311,7 +311,7 @@ static int extract_single_file(struct archive *archive, size_t backup_len = strlen(oldbackup) + 34; MALLOC(backup, backup_len, RET_ERR(PM_ERR_MEMORY, -1));
- sprintf(backup, "%s\t%s", oldbackup, hash_pkg); + snprintf(backup, backup_len, "%s\t%s", oldbackup, hash_pkg);
I don't think this provides us any extra safety, because we already compute exactly the size of the destination and allocate that memory. I think it makes more sense where we define a fixed array, like char path[PATH_MAX], and we then copy strings of different size to it : src/pacman/pacman.c: snprintf(path, PATH_MAX, "%s%s", alpm_option_get_root(), DBPATH + 1); src/pacman/pacman.c: snprintf(path, PATH_MAX, "%s%s", alpm_option_get_root(), LOGFILE + 1);
On Sun, Oct 18, 2009 at 5:02 AM, Xavier <shiningxc@gmail.com> wrote:
On Sun, Oct 18, 2009 at 6:03 AM, Laszlo Papp <djszapi2@gmail.com> wrote:
* Size examined str* function usage is a common coding practice, because it's more safer to avoid breakage while using str* functions.
Signed-off-by: Laszlo Papp <djszapi@archlinux.us> --- lib/libalpm/add.c | 4 ++-- lib/libalpm/be_files.c | 10 +++++----- lib/libalpm/conflict.c | 2 +- lib/libalpm/db.c | 4 ++-- lib/libalpm/trans.c | 2 +- lib/libalpm/util.c | 16 +++++++++------- src/pacman/sync.c | 2 +- src/pacman/util.c | 2 +- 8 files changed, 22 insertions(+), 20 deletions(-)
diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index ebcd6a5..209c38e 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -311,7 +311,7 @@ static int extract_single_file(struct archive *archive, size_t backup_len = strlen(oldbackup) + 34; MALLOC(backup, backup_len, RET_ERR(PM_ERR_MEMORY, -1));
- sprintf(backup, "%s\t%s", oldbackup, hash_pkg); + snprintf(backup, backup_len, "%s\t%s", oldbackup, hash_pkg);
I don't think this provides us any extra safety, because we already compute exactly the size of the destination and allocate that memory.
I think it makes more sense where we define a fixed array, like char path[PATH_MAX], and we then copy strings of different size to it : src/pacman/pacman.c: snprintf(path, PATH_MAX, "%s%s", alpm_option_get_root(), DBPATH + 1); src/pacman/pacman.c: snprintf(path, PATH_MAX, "%s%s", alpm_option_get_root(), LOGFILE + 1);
There is zero added safety to this. All of the patches that have been coming in are relatively bogus. We've done strlen() in nearly all cases anyway. The "n" family of operations also silently truncates. This is probably worse off for us, as it could lead to silent failures. -Dan
participants (3)
-
Dan McGee
-
Laszlo Papp
-
Xavier