[pacman-dev] [PATCH 0/5] Properly detect conflicts with directory symlinks
This series of patches make pacman perform conflict checking with all directory paths resovled. This means that inter-package conflicts such as /lib/foo and /usr/lib/foo when /lib -> usr/lib are detected. Similarly, if a package moves a file from /lib/foo to /usr/lib/foo when /lib -> usr/lib (i.e. the file is effectively the same), this is no longer detected as a conflict with the filesystem. A big thanks to Andrew for fixing my initial attempt at the resolving of the filelist and making my idea for efficiently storing paths that are unchanged by resolving a reality. Allan McRae (3): Add resolved_path to alpm_filelist_t Resolve file paths during inter-package conflict check Avoid upgrade conflict with unchanged effective path Andrew Gregory (2): Add _alpm_filelist_resolve _alpm_filelist_resolve: use original filenames where possible lib/libalpm/alpm.h | 1 + lib/libalpm/conflict.c | 6 ++ lib/libalpm/filelist.c | 174 ++++++++++++++++++++++++++++++++--- lib/libalpm/filelist.h | 2 + lib/libalpm/package.c | 22 ++++- test/pacman/tests/fileconflict001.py | 2 - test/pacman/tests/fileconflict013.py | 2 - test/pacman/tests/fileconflict016.py | 2 - test/pacman/tests/fileconflict017.py | 26 ++++++ 9 files changed, 217 insertions(+), 20 deletions(-) create mode 100644 test/pacman/tests/fileconflict017.py -- 1.7.11.3
Add an array to hold the resolved paths of the files in alpm_filelist_t. When the file name and its resolved file name are identical, the pointer to the original file name is used to avoid duplicate memory allocation. Signed-off-by: Allan McRae <allan@archlinux.org> --- lib/libalpm/alpm.h | 1 + lib/libalpm/package.c | 11 ++++++++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index 9aa6ac2..c341b17 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -213,6 +213,7 @@ typedef struct _alpm_file_t { typedef struct _alpm_filelist_t { size_t count; alpm_file_t *files; + char **resolved_path; } alpm_filelist_t; /** Local package or package file backup entry */ diff --git a/lib/libalpm/package.c b/lib/libalpm/package.c index a4c3309..ab84329 100644 --- a/lib/libalpm/package.c +++ b/lib/libalpm/package.c @@ -540,6 +540,9 @@ int _alpm_pkg_dup(alpm_pkg_t *pkg, alpm_pkg_t **new_ptr) } } newpkg->files.count = pkg->files.count; + /* deliberately do not copy resolved_path as this is only used + * during conflict checking and the sorting of list does not readily + * allow keeping its efficient memory usage when copying */ } /* internal */ @@ -590,9 +593,15 @@ void _alpm_pkg_free(alpm_pkg_t *pkg) if(pkg->files.count) { size_t i; for(i = 0; i < pkg->files.count; i++) { - free(pkg->files.files[i].name); + FREE(pkg->files.files[i].name); } free(pkg->files.files); + if(pkg->files.resolved_path) { + for(i = 0; i < pkg->files.count; i++) { + free(pkg->files.resolved_path[i]); + } + free(pkg->files.resolved_path); + } } alpm_list_free_inner(pkg->backup, (alpm_list_fn_free)_alpm_backup_free); alpm_list_free(pkg->backup); -- 1.7.11.3
From: Andrew Gregory <andrew.gregory.8@gmail.com> The _alpm_filelist_resolve function takes a filelist and creates a list with any symlinks in directory paths resolved. Signed-off-by: Allan McRae <allan@archlinux.org> --- lib/libalpm/filelist.c | 156 +++++++++++++++++++++++++++++++++++++++++++++++++ lib/libalpm/filelist.h | 2 + 2 files changed, 158 insertions(+) diff --git a/lib/libalpm/filelist.c b/lib/libalpm/filelist.c index 1928056..d32a3e5 100644 --- a/lib/libalpm/filelist.c +++ b/lib/libalpm/filelist.c @@ -17,10 +17,166 @@ * along with this program. If not, see <http://www.gnu.org/licenses/>. */ +#include <limits.h> #include <string.h> +#include <sys/stat.h> /* libalpm */ #include "filelist.h" +#include "util.h" + +/** Helper function for comparing strings when sorting */ +static int _strcmp(const void *s1, const void *s2) +{ + return strcmp(*(char **)s1, *(char **)s2); +} + +/** + * @brief Resolves a symlink and its children. + * + * @attention Pre-condition: files must be sorted! + * + * @param files filelist to resolve + * @param i index in files to start processing + * @param path absolute path for the symlink being resolved + * @param root_len length of the root portion of path + * @param resolving is file \i in \files a symlink that needs to be resolved + * + * @return the index of the last file resolved + */ +size_t _alpm_filelist_resolve_link( + alpm_filelist_t *files, size_t i, char *path, size_t root_len, int resolving) +{ + struct stat sbuf; + const char *causal_dir; /* symlink being resolved */ + char *filename_r; /* resolved filename */ + size_t causal_dir_len = 0, causal_dir_r_len = 0; + + if(resolving) { + /* deal with the symlink being resolved */ + filename_r = malloc(PATH_MAX); + causal_dir = files->files[i].name; + causal_dir_len = strlen(causal_dir); + if(realpath(path, filename_r) == NULL) { + files->resolved_path[i] = strdup(causal_dir); + return i; + } + causal_dir_r_len = strlen(filename_r + root_len) + 1; + if(causal_dir_r_len >= PATH_MAX) { + files->resolved_path[i] = strdup(causal_dir); + return i; + } + /* remove root_r from filename_r */ + memmove(filename_r, filename_r + root_len, causal_dir_r_len + 1); + strcpy(filename_r + causal_dir_r_len - 1, "/"); + files->resolved_path[i] = strdup(filename_r); + i++; + } + + for(; i < files->count && (!resolving || + strncmp(files->files[i].name, causal_dir, causal_dir_len) == 0); i++) { + char *filename = files->files[i].name; + size_t f_len = strlen(filename); + + if(resolving) { + if(f_len + causal_dir_r_len - causal_dir_len > PATH_MAX) { + files->resolved_path[i] = strdup(filename); + continue; + } + + strcpy(filename_r + causal_dir_r_len, filename + causal_dir_len); + + if(root_len + causal_dir_r_len + f_len - causal_dir_len > PATH_MAX) { + /* absolute path is too long */ + files->resolved_path[i] = strdup(filename_r); + continue; + } + } else { + filename_r = filename; + } + + /* deal with files and paths too long to resolve*/ + if(filename[f_len-1] != '/') { + files->resolved_path[i] = strdup(filename_r); + continue; + } + + /* construct absolute path and stat() */ + strcpy(path + root_len, filename_r); + int exists = !_alpm_lstat(path, &sbuf); + + /* deal with symlinks */ + if(exists && S_ISLNK(sbuf.st_mode)) { + i = _alpm_filelist_resolve_link(files, i, path, root_len, 1); + continue; + } + + /* deal with normal directories */ + files->resolved_path[i] = strdup(filename_r); + + /* deal with children of non-existent directories to reduce lstat() calls */ + if (!exists) { + char *f; + i++; + while(i < files->count && strncmp(files->files[i].name, filename, f_len) == 0) { + f = files->files[i].name; + if(resolving && strlen(f + causal_dir_len) + causal_dir_r_len <= PATH_MAX) { + strcpy(filename_r + causal_dir_r_len, f + causal_dir_len); + files->resolved_path[i] = strdup(filename_r); + } else { + files->resolved_path[i] = strdup(f); + } + i++; + } + i--; + } + } + + if(resolving) { + free(filename_r); + } + + return i-1; +} + +/** + * @brief Takes a file list and resolves all directory paths according to filesystem + * + * @attention Pre-condition: files must be sorted! + * + * @note A symlink and directory at the same path in two difference packages + * causes a conflict so the filepath can not change as packages get installed + * + * @param handle the context handle + * @param files list of files to resolve + */ +void _alpm_filelist_resolve(alpm_handle_t *handle, alpm_filelist_t *files) +{ + char path[PATH_MAX]; + size_t root_len; + + if(!files || files->resolved_path) { + return; + } + + MALLOC(files->resolved_path, files->count * sizeof(char*), return); + memset(files->resolved_path, 0, files->count); + + /* not much point in going on if we can't even resolve root */ + if(realpath(handle->root, path) == NULL){ + return; + } + root_len = strlen(path) + 1; + if(root_len >= PATH_MAX) { + return; + } + strcpy(path + root_len - 1, "/"); + + _alpm_filelist_resolve_link(files, 0, path, root_len, 0); + + qsort(files->resolved_path, files->count, sizeof(char *), _strcmp); +} + /* Returns the difference of the provided two lists of files. * Pre-condition: both lists are sorted! diff --git a/lib/libalpm/filelist.h b/lib/libalpm/filelist.h index 2d5cbc0..3152b9d 100644 --- a/lib/libalpm/filelist.h +++ b/lib/libalpm/filelist.h @@ -21,6 +21,8 @@ #include "alpm.h" +size_t _alpm_filelist_resolve_link(alpm_filelist_t *files, size_t i, char *path, size_t root_len, int resolving); +void _alpm_filelist_resolve(alpm_handle_t *handle, alpm_filelist_t *files); alpm_list_t *_alpm_filelist_difference(alpm_filelist_t *filesA, alpm_filelist_t *filesB); -- 1.7.11.3
On Sun, Jul 29, 2012 at 04:18:32PM +1000, Allan McRae wrote:
I'm worried about symbol clashes here, since technically '_' isn't our namespace. Please pick a name similar to what we use elsewhere for comparison callbacks, such as _alpm_str_cmp or _alpm_files_cmp.
This seems very prone to leakage. Why not just allocate it on the stack?
STRDUP macro?
STRDUP...
STRDUP... I'm done pointing these out, but there's more.
This seems really ugly. Can't the second half of the exit condition be moved into the body of the loop?
Are you sure this never copies more than PATH_MAX bytes into filename_r? There's more of these buffer overflow concerns that flat out aren't handled.
Style nit -- add space around the '-', and a space before the close of the comment. I'm not really sure how this does what the comment says.
Just use CALLOC instead of MALLOC+memset
Why not use direct assignment or memcpy here?
Wrap at 80 columns, please.
On Sun, 29 Jul 2012 09:50:13 -0400 Dave Reisner <d@falconindy.com> wrote:
filename_r is only needed if a symlink is being resolved, so most of the time allocating memory for it would be a waste. It also helps prevent a stack overflow since this function is recursive. You are right about the leakage though, as evidenced a whole 5 lines below... I suppose it should be changed to the MALLOC macro though; I keep forgetting about those things.
Unless I got the math wrong, the if directly above ensures that very thing. All of the strcpy()s, with the notable exception of the one directly below, have similar checks.
Probably because I erroneously moved the length check above when refactoring...
The _alpm_filelist_resolve function takes a filelist and creates a list with any symlinks in directory paths resolved. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- Made changes according to Dave's suggestions. Also made some minor adjustments to bring it better into line with the coding standards and hopefully make it more readable. lib/libalpm/filelist.c | 178 +++++++++++++++++++++++++++++++++++++++++++++++++ lib/libalpm/filelist.h | 3 + 2 files changed, 181 insertions(+) diff --git a/lib/libalpm/filelist.c b/lib/libalpm/filelist.c index 1928056..783c8bf 100644 --- a/lib/libalpm/filelist.c +++ b/lib/libalpm/filelist.c @@ -17,10 +17,188 @@ * along with this program. If not, see <http://www.gnu.org/licenses/>. */ +#include <limits.h> #include <string.h> +#include <sys/stat.h> /* libalpm */ #include "filelist.h" +#include "util.h" + +/** Helper function for comparing strings when sorting */ +static int _alpm_filelist_strcmp(const void *s1, const void *s2) +{ + return strcmp(*(char **)s1, *(char **)s2); +} + +/** + * @brief Resolves a symlink and its children. + * + * @attention Pre-condition: files must be sorted! + * + * @param files filelist to resolve + * @param i index in files to start processing + * @param path absolute path for the symlink being resolved + * @param root_len length of the root portion of path + * @param resolving is file \i in \files a symlink that needs to be resolved + * + * @return the index of the last file resolved + */ +size_t _alpm_filelist_resolve_link( + alpm_filelist_t *files, size_t i, char *path, size_t root_len, int resolving) +{ + char *causal_dir; /* symlink being resolved */ + char *filename_r = NULL; /* resolved filename */ + size_t causal_dir_len = 0, causal_dir_r_len = 0; + + if(resolving) { + /* deal with the symlink being resolved */ + MALLOC(filename_r, PATH_MAX, goto error); + causal_dir = files->files[i].name; + causal_dir_len = strlen(causal_dir); + if(realpath(path, filename_r) == NULL) { + STRDUP(files->resolved_path[i], causal_dir, goto error); + FREE(filename_r); + return i; + } + causal_dir_r_len = strlen(filename_r + root_len) + 1; + if(causal_dir_r_len >= PATH_MAX) { + STRDUP(files->resolved_path[i], causal_dir, goto error); + FREE(filename_r); + return i; + } + /* remove root_r from filename_r */ + memmove(filename_r, filename_r + root_len, causal_dir_r_len); + filename_r[causal_dir_r_len - 1] = '/'; + filename_r[causal_dir_r_len] = '\0'; + STRDUP(files->resolved_path[i], filename_r, goto error); + i++; + } + + for(; i < files->count; i++) { + char *filename = files->files[i].name; + size_t filename_len = strlen(filename); + size_t filename_r_len = filename_len; + struct stat sbuf; + int exists; + + if(resolving) { + if(filename_len < causal_dir_len || strncmp(filename, causal_dir, causal_dir_len) != 0) { + /* not inside causal_dir anymore */ + break; + } + + filename_r_len = filename_len + causal_dir_r_len - causal_dir_len; + if(filename_r_len >= PATH_MAX) { + /* resolved path is too long */ + STRDUP(files->resolved_path[i], filename, goto error); + continue; + } + + strcpy(filename_r + causal_dir_r_len, filename + causal_dir_len); + } else { + filename_r = filename; + } + + /* deal with files and paths too long to resolve*/ + if(filename[filename_len - 1] != '/' || root_len + filename_r_len >= PATH_MAX) { + STRDUP(files->resolved_path[i], filename_r, goto error); + continue; + } + + /* construct absolute path and stat() */ + strcpy(path + root_len, filename_r); + exists = !_alpm_lstat(path, &sbuf); + + /* deal with symlinks */ + if(exists && S_ISLNK(sbuf.st_mode)) { + i = _alpm_filelist_resolve_link(files, i, path, root_len, 1); + continue; + } + + /* deal with normal directories */ + STRDUP(files->resolved_path[i], filename_r, goto error); + + /* deal with children of non-existent directories to reduce lstat() calls */ + if (!exists) { + for(i++; i < files->count; i++) { + char *f = files->files[i].name;; + size_t f_len = strlen(f); + size_t f_r_len; + + if(f_len < filename_len || strncmp(f, filename, filename_len) != 0) { + /* not inside the non-existent dir anymore */ + break; + } + + f_r_len = f_len + causal_dir_r_len - causal_dir_len; + if(resolving && f_r_len <= PATH_MAX) { + strcpy(filename_r + causal_dir_r_len, f + causal_dir_len); + STRDUP(files->resolved_path[i], filename_r, goto error); + } else { + STRDUP(files->resolved_path[i], f, goto error); + } + } + i--; + } + } + + if(resolving) { + FREE(filename_r); + } + + return i-1; + +error: + if(resolving) { + FREE(filename_r); + } + /* out of memory, not much point in going on */ + return files->count; +} + +/** + * @brief Takes a file list and resolves all directory paths according to the + * filesystem + * + * @attention Pre-condition: files must be sorted! + * + * @note A symlink and directory at the same path in two difference packages + * causes a conflict so the filepath can not change as packages get installed + * + * @param handle the context handle + * @param files list of files to resolve + */ +void _alpm_filelist_resolve(alpm_handle_t *handle, alpm_filelist_t *files) +{ + char path[PATH_MAX]; + size_t root_len; + + if(!files || files->resolved_path) { + return; + } + + CALLOC(files->resolved_path, files->count, sizeof(char *), return); + + /* not much point in going on if we can't even resolve root */ + if(realpath(handle->root, path) == NULL){ + return; + } + root_len = strlen(path) + 1; + if(root_len >= PATH_MAX) { + return; + } + path[root_len - 1] = '/'; + path[root_len] = '\0'; + + _alpm_filelist_resolve_link(files, 0, path, root_len, 0); + + qsort(files->resolved_path, files->count, sizeof(char *), + _alpm_filelist_strcmp); + + return; +} + /* Returns the difference of the provided two lists of files. * Pre-condition: both lists are sorted! diff --git a/lib/libalpm/filelist.h b/lib/libalpm/filelist.h index 2d5cbc0..ef865da 100644 --- a/lib/libalpm/filelist.h +++ b/lib/libalpm/filelist.h @@ -21,6 +21,9 @@ #include "alpm.h" +size_t _alpm_filelist_resolve_link(alpm_filelist_t *files, size_t i, + char *path, size_t root_len, int resolving); +void _alpm_filelist_resolve(alpm_handle_t *handle, alpm_filelist_t *files); alpm_list_t *_alpm_filelist_difference(alpm_filelist_t *filesA, alpm_filelist_t *filesB); -- 1.7.11.4
From: Andrew Gregory <andrew.gregory.8@gmail.com> If a filename isn't resolved, the original can be used instead of strdup()ing it. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/filelist.c | 8 ++++---- lib/libalpm/package.c | 19 +++++++++++++------ 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/lib/libalpm/filelist.c b/lib/libalpm/filelist.c index d32a3e5..288907a 100644 --- a/lib/libalpm/filelist.c +++ b/lib/libalpm/filelist.c @@ -80,7 +80,7 @@ size_t _alpm_filelist_resolve_link( if(resolving) { if(f_len + causal_dir_r_len - causal_dir_len > PATH_MAX) { - files->resolved_path[i] = strdup(filename); + files->resolved_path[i] = filename; continue; } @@ -97,7 +97,7 @@ size_t _alpm_filelist_resolve_link( /* deal with files and paths too long to resolve*/ if(filename[f_len-1] != '/') { - files->resolved_path[i] = strdup(filename_r); + files->resolved_path[i] = resolving ? strdup(filename_r) : filename; continue; } @@ -112,7 +112,7 @@ size_t _alpm_filelist_resolve_link( } /* deal with normal directories */ - files->resolved_path[i] = strdup(filename_r); + files->resolved_path[i] = resolving ? strdup(filename_r) : filename; /* deal with children of non-existent directories to reduce lstat() calls */ if (!exists) { @@ -124,7 +124,7 @@ size_t _alpm_filelist_resolve_link( strcpy(filename_r + causal_dir_r_len, f + causal_dir_len); files->resolved_path[i] = strdup(filename_r); } else { - files->resolved_path[i] = strdup(f); + files->resolved_path[i] = f; } i++; } diff --git a/lib/libalpm/package.c b/lib/libalpm/package.c index ab84329..4887e21 100644 --- a/lib/libalpm/package.c +++ b/lib/libalpm/package.c @@ -591,17 +591,24 @@ void _alpm_pkg_free(alpm_pkg_t *pkg) free_deplist(pkg->replaces); FREELIST(pkg->groups); if(pkg->files.count) { - size_t i; - for(i = 0; i < pkg->files.count; i++) { - FREE(pkg->files.files[i].name); - } - free(pkg->files.files); + size_t i, j, k; if(pkg->files.resolved_path) { - for(i = 0; i < pkg->files.count; i++) { + for(i = 0, j = 0; i < pkg->files.count; i++) { + for(k = j; k <= pkg->files.count; k++) { + if(pkg->files.resolved_path[i] == pkg->files.files[k].name) { + pkg->files.files[k].name = NULL; + j = k + 1; + break; + } + } free(pkg->files.resolved_path[i]); } free(pkg->files.resolved_path); } + for(j = 0; j < pkg->files.count; j++) { + FREE(pkg->files.files[j].name); + } + free(pkg->files.files); } alpm_list_free_inner(pkg->backup, (alpm_list_fn_free)_alpm_backup_free); alpm_list_free(pkg->backup); -- 1.7.11.3
If a filename isn't resolved, the original can be used instead of strdup()ing it. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- Modified to account for changes to the previous patch. lib/libalpm/filelist.c | 37 +++++++++++++++++++++---------------- lib/libalpm/package.c | 19 +++++++++++++------ 2 files changed, 34 insertions(+), 22 deletions(-) diff --git a/lib/libalpm/filelist.c b/lib/libalpm/filelist.c index 783c8bf..58b1971 100644 --- a/lib/libalpm/filelist.c +++ b/lib/libalpm/filelist.c @@ -57,13 +57,13 @@ size_t _alpm_filelist_resolve_link( causal_dir = files->files[i].name; causal_dir_len = strlen(causal_dir); if(realpath(path, filename_r) == NULL) { - STRDUP(files->resolved_path[i], causal_dir, goto error); + files->resolved_path[i] = causal_dir; FREE(filename_r); return i; } causal_dir_r_len = strlen(filename_r + root_len) + 1; if(causal_dir_r_len >= PATH_MAX) { - STRDUP(files->resolved_path[i], causal_dir, goto error); + files->resolved_path[i] = causal_dir; FREE(filename_r); return i; } @@ -91,23 +91,25 @@ size_t _alpm_filelist_resolve_link( filename_r_len = filename_len + causal_dir_r_len - causal_dir_len; if(filename_r_len >= PATH_MAX) { /* resolved path is too long */ - STRDUP(files->resolved_path[i], filename, goto error); + files->resolved_path[i] = filename; continue; } strcpy(filename_r + causal_dir_r_len, filename + causal_dir_len); - } else { - filename_r = filename; } /* deal with files and paths too long to resolve*/ if(filename[filename_len - 1] != '/' || root_len + filename_r_len >= PATH_MAX) { - STRDUP(files->resolved_path[i], filename_r, goto error); + if(resolving) { + STRDUP(files->resolved_path[i], filename_r, goto error); + } else { + files->resolved_path[i] = filename; + } continue; } /* construct absolute path and stat() */ - strcpy(path + root_len, filename_r); + strcpy(path + root_len, resolving ? filename_r : filename); exists = !_alpm_lstat(path, &sbuf); /* deal with symlinks */ @@ -117,7 +119,11 @@ size_t _alpm_filelist_resolve_link( } /* deal with normal directories */ - STRDUP(files->resolved_path[i], filename_r, goto error); + if(resolving) { + STRDUP(files->resolved_path[i], filename_r, goto error); + } else { + files->resolved_path[i] = filename; + } /* deal with children of non-existent directories to reduce lstat() calls */ if (!exists) { @@ -136,25 +142,24 @@ size_t _alpm_filelist_resolve_link( strcpy(filename_r + causal_dir_r_len, f + causal_dir_len); STRDUP(files->resolved_path[i], filename_r, goto error); } else { - STRDUP(files->resolved_path[i], f, goto error); + files->resolved_path[i] = f; } } i--; } } - if(resolving) { - FREE(filename_r); - } + FREE(filename_r); return i-1; error: - if(resolving) { - FREE(filename_r); + FREE(filename_r); + /* out of memory, set remaining files to their original names */ + for(; i < files->count; (i)++) { + files->resolved_path[i] = files->files[i].name; } - /* out of memory, not much point in going on */ - return files->count; + return i-1; } /** diff --git a/lib/libalpm/package.c b/lib/libalpm/package.c index ab84329..4887e21 100644 --- a/lib/libalpm/package.c +++ b/lib/libalpm/package.c @@ -591,17 +591,24 @@ void _alpm_pkg_free(alpm_pkg_t *pkg) free_deplist(pkg->replaces); FREELIST(pkg->groups); if(pkg->files.count) { - size_t i; - for(i = 0; i < pkg->files.count; i++) { - FREE(pkg->files.files[i].name); - } - free(pkg->files.files); + size_t i, j, k; if(pkg->files.resolved_path) { - for(i = 0; i < pkg->files.count; i++) { + for(i = 0, j = 0; i < pkg->files.count; i++) { + for(k = j; k <= pkg->files.count; k++) { + if(pkg->files.resolved_path[i] == pkg->files.files[k].name) { + pkg->files.files[k].name = NULL; + j = k + 1; + break; + } + } free(pkg->files.resolved_path[i]); } free(pkg->files.resolved_path); } + for(j = 0; j < pkg->files.count; j++) { + FREE(pkg->files.files[j].name); + } + free(pkg->files.files); } alpm_list_free_inner(pkg->backup, (alpm_list_fn_free)_alpm_backup_free); alpm_list_free(pkg->backup); -- 1.7.11.4
Return -1 if a path is too long to resolve or we run out of memory. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/filelist.c | 81 ++++++++++++++++++++++++++++---------------------- lib/libalpm/filelist.h | 4 +-- 2 files changed, 47 insertions(+), 38 deletions(-) diff --git a/lib/libalpm/filelist.c b/lib/libalpm/filelist.c index 58b1971..bf390ab 100644 --- a/lib/libalpm/filelist.c +++ b/lib/libalpm/filelist.c @@ -31,21 +31,25 @@ static int _alpm_filelist_strcmp(const void *s1, const void *s2) return strcmp(*(char **)s1, *(char **)s2); } +/* TODO make sure callers check the return value so we can bail on errors. + * For now we soldier on as best we can, skipping paths that are too long to + * resolve and using the original filenames on memory errors. */ /** * @brief Resolves a symlink and its children. * * @attention Pre-condition: files must be sorted! * * @param files filelist to resolve - * @param i index in files to start processing + * @param i pointer to the index in files to start processing, will point to + * the last file processed on return * @param path absolute path for the symlink being resolved * @param root_len length of the root portion of path * @param resolving is file \i in \files a symlink that needs to be resolved * - * @return the index of the last file resolved + * @return 0 on success, -1 on error */ -size_t _alpm_filelist_resolve_link( - alpm_filelist_t *files, size_t i, char *path, size_t root_len, int resolving) +int _alpm_filelist_resolve_link(alpm_filelist_t *files, size_t *i, + char *path, size_t root_len, int resolving) { char *causal_dir; /* symlink being resolved */ char *filename_r = NULL; /* resolved filename */ @@ -54,29 +58,29 @@ size_t _alpm_filelist_resolve_link( if(resolving) { /* deal with the symlink being resolved */ MALLOC(filename_r, PATH_MAX, goto error); - causal_dir = files->files[i].name; + causal_dir = files->files[*i].name; causal_dir_len = strlen(causal_dir); if(realpath(path, filename_r) == NULL) { - files->resolved_path[i] = causal_dir; + files->resolved_path[*i] = causal_dir; FREE(filename_r); - return i; + return -1; } causal_dir_r_len = strlen(filename_r + root_len) + 1; if(causal_dir_r_len >= PATH_MAX) { - files->resolved_path[i] = causal_dir; + files->resolved_path[*i] = causal_dir; FREE(filename_r); - return i; + return -1; } /* remove root_r from filename_r */ memmove(filename_r, filename_r + root_len, causal_dir_r_len); filename_r[causal_dir_r_len - 1] = '/'; filename_r[causal_dir_r_len] = '\0'; - STRDUP(files->resolved_path[i], filename_r, goto error); - i++; + STRDUP(files->resolved_path[*i], filename_r, goto error); + (*i)++; } - for(; i < files->count; i++) { - char *filename = files->files[i].name; + for(; *i < files->count; (*i)++) { + char *filename = files->files[*i].name; size_t filename_len = strlen(filename); size_t filename_r_len = filename_len; struct stat sbuf; @@ -91,7 +95,7 @@ size_t _alpm_filelist_resolve_link( filename_r_len = filename_len + causal_dir_r_len - causal_dir_len; if(filename_r_len >= PATH_MAX) { /* resolved path is too long */ - files->resolved_path[i] = filename; + files->resolved_path[*i] = filename; continue; } @@ -101,9 +105,9 @@ size_t _alpm_filelist_resolve_link( /* deal with files and paths too long to resolve*/ if(filename[filename_len - 1] != '/' || root_len + filename_r_len >= PATH_MAX) { if(resolving) { - STRDUP(files->resolved_path[i], filename_r, goto error); + STRDUP(files->resolved_path[*i], filename_r, goto error); } else { - files->resolved_path[i] = filename; + files->resolved_path[*i] = filename; } continue; } @@ -114,21 +118,21 @@ size_t _alpm_filelist_resolve_link( /* deal with symlinks */ if(exists && S_ISLNK(sbuf.st_mode)) { - i = _alpm_filelist_resolve_link(files, i, path, root_len, 1); + _alpm_filelist_resolve_link(files, i, path, root_len, 1); continue; } /* deal with normal directories */ if(resolving) { - STRDUP(files->resolved_path[i], filename_r, goto error); + STRDUP(files->resolved_path[*i], filename_r, goto error); } else { - files->resolved_path[i] = filename; + files->resolved_path[*i] = filename; } /* deal with children of non-existent directories to reduce lstat() calls */ if (!exists) { - for(i++; i < files->count; i++) { - char *f = files->files[i].name;; + for((*i)++; *i < files->count; (*i)++) { + char *f = files->files[*i].name; size_t f_len = strlen(f); size_t f_r_len; @@ -140,26 +144,28 @@ size_t _alpm_filelist_resolve_link( f_r_len = f_len + causal_dir_r_len - causal_dir_len; if(resolving && f_r_len <= PATH_MAX) { strcpy(filename_r + causal_dir_r_len, f + causal_dir_len); - STRDUP(files->resolved_path[i], filename_r, goto error); + STRDUP(files->resolved_path[*i], filename_r, goto error); } else { - files->resolved_path[i] = f; + files->resolved_path[*i] = f; } } - i--; + (*i)--; } } + (*i)--; FREE(filename_r); - return i-1; + return 0; error: FREE(filename_r); /* out of memory, set remaining files to their original names */ - for(; i < files->count; (i)++) { - files->resolved_path[i] = files->files[i].name; + for(; *i < files->count; (*i)++) { + files->resolved_path[*i] = files->files[*i].name; } - return i-1; + (*i)--; + return -1; } /** @@ -173,35 +179,38 @@ error: * * @param handle the context handle * @param files list of files to resolve + * + * @return 0 on success, -1 on error */ -void _alpm_filelist_resolve(alpm_handle_t *handle, alpm_filelist_t *files) +int _alpm_filelist_resolve(alpm_handle_t *handle, alpm_filelist_t *files) { char path[PATH_MAX]; - size_t root_len; + size_t root_len, i=0; + int ret = 0; if(!files || files->resolved_path) { - return; + return 0; } - CALLOC(files->resolved_path, files->count, sizeof(char *), return); + CALLOC(files->resolved_path, files->count, sizeof(char *), return -1); /* not much point in going on if we can't even resolve root */ if(realpath(handle->root, path) == NULL){ - return; + return -1; } root_len = strlen(path) + 1; if(root_len >= PATH_MAX) { - return; + return -1; } path[root_len - 1] = '/'; path[root_len] = '\0'; - _alpm_filelist_resolve_link(files, 0, path, root_len, 0); + ret = _alpm_filelist_resolve_link(files, &i, path, root_len, 0); qsort(files->resolved_path, files->count, sizeof(char *), _alpm_filelist_strcmp); - return; + return ret; } diff --git a/lib/libalpm/filelist.h b/lib/libalpm/filelist.h index ef865da..8c4f8eb 100644 --- a/lib/libalpm/filelist.h +++ b/lib/libalpm/filelist.h @@ -21,9 +21,9 @@ #include "alpm.h" -size_t _alpm_filelist_resolve_link(alpm_filelist_t *files, size_t i, +int _alpm_filelist_resolve_link(alpm_filelist_t *files, size_t *i, char *path, size_t root_len, int resolving); -void _alpm_filelist_resolve(alpm_handle_t *handle, alpm_filelist_t *files); +int _alpm_filelist_resolve(alpm_handle_t *handle, alpm_filelist_t *files); alpm_list_t *_alpm_filelist_difference(alpm_filelist_t *filesA, alpm_filelist_t *filesB); -- 1.7.11.4
File paths are resolved if necessary during inter-package conflict checks so that packages carrying the same effective file due to directory symlinks on the filesystem are flagged as conflicting. Signed-off-by: Allan McRae <allan@archlinux.org> --- lib/libalpm/conflict.c | 5 +++++ lib/libalpm/filelist.c | 13 +++++-------- test/pacman/tests/fileconflict001.py | 2 -- test/pacman/tests/fileconflict016.py | 2 -- test/pacman/tests/fileconflict017.py | 26 ++++++++++++++++++++++++++ 5 files changed, 36 insertions(+), 12 deletions(-) create mode 100644 test/pacman/tests/fileconflict017.py diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c index ccfe990..afef56c 100644 --- a/lib/libalpm/conflict.c +++ b/lib/libalpm/conflict.c @@ -361,12 +361,17 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, int percent = (current * 100) / numtargs; PROGRESS(handle, ALPM_PROGRESS_CONFLICTS_START, "", percent, numtargs, current); + + _alpm_filelist_resolve(handle, alpm_pkg_get_files(p1)); + /* CHECK 1: check every target against every target */ _alpm_log(handle, ALPM_LOG_DEBUG, "searching for file conflicts: %s\n", p1->name); for(j = i->next; j; j = j->next) { alpm_list_t *common_files; alpm_pkg_t *p2 = j->data; + _alpm_filelist_resolve(handle, alpm_pkg_get_files(p2)); + common_files = _alpm_filelist_intersection(alpm_pkg_get_files(p1), alpm_pkg_get_files(p2)); diff --git a/lib/libalpm/filelist.c b/lib/libalpm/filelist.c index 288907a..59bf1ec 100644 --- a/lib/libalpm/filelist.c +++ b/lib/libalpm/filelist.c @@ -241,21 +241,18 @@ alpm_list_t *_alpm_filelist_intersection(alpm_filelist_t *filesA, int cmp, isdirA, isdirB; char *strA, *strB; - alpm_file_t *fileA = filesA->files + ctrA; - alpm_file_t *fileB = filesB->files + ctrB; - isdirA = 0; - strA = fileA->name; + strA = filesA->resolved_path[ctrA]; if(strA[strlen(strA)-1] == '/') { isdirA = 1; - strA = strndup(fileA->name, strlen(strA)-1); + strA = strndup(filesA->resolved_path[ctrA], strlen(strA)-1); } isdirB = 0; - strB = fileB->name; + strB = filesB->resolved_path[ctrB]; if(strB[strlen(strB)-1] == '/') { isdirB = 1; - strB = strndup(fileB->name, strlen(strB)-1); + strB = strndup(filesB->resolved_path[ctrB], strlen(strB)-1); } cmp = strcmp(strA, strB); @@ -273,7 +270,7 @@ alpm_list_t *_alpm_filelist_intersection(alpm_filelist_t *filesA, /* when not directories, item in both qualifies as an intersect */ if(! (isdirA && isdirB)) { - ret = alpm_list_add(ret, fileA); + ret = alpm_list_add(ret, filesA->files + ctrA); } ctrA++; ctrB++; diff --git a/test/pacman/tests/fileconflict001.py b/test/pacman/tests/fileconflict001.py index e1371f8..b1ad5e1 100644 --- a/test/pacman/tests/fileconflict001.py +++ b/test/pacman/tests/fileconflict001.py @@ -25,5 +25,3 @@ self.addrule("!PKG_EXIST=pkg2") self.addrule("FILE_EXIST=dir/realdir/realfile") self.addrule("!FILE_EXIST=dir/realdir/file") - -self.expectfailure = True diff --git a/test/pacman/tests/fileconflict016.py b/test/pacman/tests/fileconflict016.py index 5625984..c5daf48 100644 --- a/test/pacman/tests/fileconflict016.py +++ b/test/pacman/tests/fileconflict016.py @@ -22,5 +22,3 @@ self.addrule("PACMAN_RETCODE=1") self.addrule("!PKG_EXIST=pkg1") self.addrule("!PKG_EXIST=pkg2") - -self.expectfailure = True diff --git a/test/pacman/tests/fileconflict017.py b/test/pacman/tests/fileconflict017.py new file mode 100644 index 0000000..3855a93 --- /dev/null +++ b/test/pacman/tests/fileconflict017.py @@ -0,0 +1,26 @@ +self.description = "file conflict with same effective path across packages (directory symlink - deep)" + +lp1 = pmpkg("filesystem", "1.0-1") +lp1.files = ["usr/", + "usr/lib/", + "lib -> usr/lib/"] +self.addpkg2db("local", lp1) + +p1 = pmpkg("pkg1") +p1.files = ["lib/", + "lib/foo/", + "lib/foo/bar"] +self.addpkg2db("sync", p1) + +p2 = pmpkg("pkg2") +p2.files = ["usr/", + "usr/lib/", + "usr/lib/foo/", + "usr/lib/foo/bar"] +self.addpkg2db("sync", p2) + +self.args = "-S pkg1 pkg2" + +self.addrule("PACMAN_RETCODE=1") +self.addrule("!PKG_EXIST=pkg1") +self.addrule("!PKG_EXIST=pkg2") -- 1.7.11.3
This applies to a case such as when /lib is a symlink to /usr/lib. If a package is installed which contains /lib/libfoo.so, pacman will complain if this package is then "fixed" to contain /usr/lib/libfoo.so. Since these have the same effective path and it exists within the same package, ignore the conflict. Fixes FS#30681. Signed-off-by: Allan McRae <allan@archlinux.org> --- lib/libalpm/conflict.c | 1 + lib/libalpm/filelist.c | 5 ++--- test/pacman/tests/fileconflict013.py | 2 -- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c index afef56c..f087ace 100644 --- a/lib/libalpm/conflict.c +++ b/lib/libalpm/conflict.c @@ -396,6 +396,7 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, _alpm_log(handle, ALPM_LOG_DEBUG, "searching for filesystem conflicts: %s\n", p1->name); dbpkg = _alpm_db_get_pkgfromcache(handle->db_local, p1->name); + _alpm_filelist_resolve(handle, alpm_pkg_get_files(dbpkg)); /* Do two different checks here. If the package is currently installed, * then only check files that are new in the new package. If the package diff --git a/lib/libalpm/filelist.c b/lib/libalpm/filelist.c index 59bf1ec..9b7b8bd 100644 --- a/lib/libalpm/filelist.c +++ b/lib/libalpm/filelist.c @@ -190,9 +190,8 @@ alpm_list_t *_alpm_filelist_difference(alpm_filelist_t *filesA, while(ctrA < filesA->count && ctrB < filesB->count) { alpm_file_t *fileA = filesA->files + ctrA; - alpm_file_t *fileB = filesB->files + ctrB; - const char *strA = fileA->name; - const char *strB = fileB->name; + const char *strA = filesA->resolved_path[ctrA]; + const char *strB = filesB->resolved_path[ctrB]; /* skip directories, we don't care about them */ if(strA[strlen(strA)-1] == '/') { ctrA++; diff --git a/test/pacman/tests/fileconflict013.py b/test/pacman/tests/fileconflict013.py index 521b62e..a83923c 100644 --- a/test/pacman/tests/fileconflict013.py +++ b/test/pacman/tests/fileconflict013.py @@ -18,5 +18,3 @@ self.addrule("PACMAN_RETCODE=0") self.addrule("PKG_VERSION=pkg1|1.0-2") - -self.expectfailure = True -- 1.7.11.3
participants (3)
-
Allan McRae
-
Andrew Gregory
-
Dave Reisner