Re: [pacman-dev] [PATCH] Do not add root prefix twice when checking database files

On 03/20/16 at 09:38pm, Allan McRae wrote:

When checking .INSTALL and .CHANGELOG files in the mtree file, we need to find the path they are stored in the local database. This was appending the root prefix twice as alpm_option_get_dbpath already includes the root path.

While fixing that issue I added checks that the paths for the database files were not longer than PATH_MAX.

Fixes FS#48563.

Signed-off-by: Allan McRae ---

For backport to 5.0.2.

src/pacman/check.c | 58 ++++++++++++++++++++++++++++++++---------------------- 1 file changed, 34 insertions(+), 24 deletions(-)

diff --git a/src/pacman/check.c b/src/pacman/check.c index 0fe74e8..86c25c8 100644 --- a/src/pacman/check.c +++ b/src/pacman/check.c @@ -254,7 +254,6 @@ int check_pkg_full(alpm_pkg_t *pkg) const char *root, *pkgname; size_t errors = 0; size_t rootlen; - char filepath[PATH_MAX]; struct archive *mtree; struct archive_entry *entry = NULL; size_t file_count = 0; @@ -267,7 +266,6 @@ int check_pkg_full(alpm_pkg_t *pkg) pm_printf(ALPM_LOG_ERROR, _("path too long: %s%s\n"), root, ""); return 1; } - strcpy(filepath, root);

pkgname = alpm_pkg_get_name(pkg); mtree = alpm_pkg_mtree_open(pkg); @@ -282,6 +280,8 @@ int check_pkg_full(alpm_pkg_t *pkg) while(alpm_pkg_mtree_next(pkg, mtree, &entry) == ARCHIVE_OK) { struct stat st; const char *path = archive_entry_pathname(entry); + char filepath[PATH_MAX]; + int filepath_len; mode_t type; size_t file_errors = 0; int backup = 0; @@ -292,32 +292,42 @@ int check_pkg_full(alpm_pkg_t *pkg) path += 2; }

- if(strcmp(path, ".INSTALL") == 0) { - char filename[PATH_MAX]; - snprintf(filename, PATH_MAX, "%slocal/%s-%s/install", - alpm_option_get_dbpath(config->handle) + 1, - pkgname, alpm_pkg_get_version(pkg)); - archive_entry_set_pathname(entry, filename); - path = archive_entry_pathname(entry); - } else if(strcmp(path, ".CHANGELOG") == 0) { - char filename[PATH_MAX]; - snprintf(filename, PATH_MAX, "%slocal/%s-%s/changelog", - alpm_option_get_dbpath(config->handle) + 1, - pkgname, alpm_pkg_get_version(pkg)); - archive_entry_set_pathname(entry, filename); - path = archive_entry_pathname(entry); - } else if(*path == '.') { - continue; + if(*path == '.') { + if(strcmp(path, ".INSTALL") == 0) { + char filename[PATH_MAX]; + snprintf(filename, PATH_MAX, "%slocal/%s-%s/install", + alpm_option_get_dbpath(config->handle) + 1, + pkgname, alpm_pkg_get_version(pkg)); + archive_entry_set_pathname(entry, filename); + path = archive_entry_pathname(entry); + } else if(strcmp(path, ".CHANGELOG") == 0) { + char filename[PATH_MAX]; + snprintf(filename, PATH_MAX, "%slocal/%s-%s/changelog", + alpm_option_get_dbpath(config->handle) + 1, + pkgname, alpm_pkg_get_version(pkg)); + archive_entry_set_pathname(entry, filename); + path = archive_entry_pathname(entry); + } else { + continue; + } + + /* Do not append root directory as alpm_option_get_dbpath already + * includes it */

This is not necessarily correct. dbpath can be *outside* of our root; the important thing is that dbpath is already absolute.

+ filepath_len = snprintf(filepath, PATH_MAX, "/%s", archive_entry_pathname(entry));

You skip the leading '/' above only to add it back here, and the above snprintf has no overflow check. I would suggest doing something similar to extract_db_file in add.c and have the above if/else chain just set the name of the dbfile and do all of the actual path manipulation here.

+ if(filepath_len >= PATH_MAX) { + pm_printf(ALPM_LOG_WARNING, _("path too long: %s%s\n"), root, path); + continue; + } + } else { + filepath_len = snprintf(filepath, PATH_MAX, "%s%s", root, path); + if(filepath_len >= PATH_MAX) { + pm_printf(ALPM_LOG_WARNING, _("path too long: %s%s\n"), root, path); + continue; + } }

file_count++;

- if(rootlen + 1 + strlen(path) > PATH_MAX) { - pm_printf(ALPM_LOG_WARNING, _("path too long: %s%s\n"), root, path); - continue; - } - strcpy(filepath + rootlen, path); - exists = check_file_exists(pkgname, filepath, rootlen, &st); if(exists == 1) { errors++; -- 2.7.3