On Sun, Jul 29, 2012 at 04:18:32PM +1000, Allan McRae wrote:
> From: Andrew Gregory <[email protected]>
>
> The _alpm_filelist_resolve function takes a filelist and creates
> a list with any symlinks in directory paths resolved.
>
> Signed-off-by: Allan McRae <[email protected]>
> ---
> 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)
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.
> +{
> + 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);
This seems very prone to leakage. Why not just allocate it on the stack?
> + 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);
STRDUP macro?
> + 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);
STRDUP...
> + 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);
STRDUP... I'm done pointing these out, but there's more.
> + i++;
> + }
> +
> + for(; i < files->count && (!resolving ||
> + strncmp(files->files[i].name, causal_dir,
> causal_dir_len) == 0); i++) {
This seems really ugly. Can't the second half of the exit condition be
moved into the body of the loop?
> + 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);
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.
> +
> + 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] != '/') {
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.
> + 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);
Just use CALLOC instead of MALLOC+memset
> +
> + /* 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, "/");
Why not use direct assignment or memcpy here?
> +
> + _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);
Wrap at 80 columns, please.
> +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
>
>
