Re: [pacman-dev] [PATCH 5/8] extract_single_file: consolidate symlink cases

On 27/04/13 10:00, Andrew Gregory wrote:

Signed-off-by: Andrew Gregory --- lib/libalpm/add.c | 33 ++++++++------------------------- 1 file changed, 8 insertions(+), 25 deletions(-)

diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index 1d9db60..0b8aedf 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -204,19 +204,17 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive, * D | 10 | 11 | 12 * * 1,2,3- extract, no magic necessary. lstat (_alpm_lstat) will fail here. - * 4,5,6,7,8- conflict checks should have caught this. either overwrite + * 4,5,6,7,8,9- conflict checks should have caught this. either overwrite * or backup the file. - * 9- follow the symlink, hopefully it is a directory, check it. - * 10- file replacing directory- don't allow it. - * 11- don't extract symlink- a dir exists here. we don't want links to - * links, etc. + * 10,11- file replacing directory- don't allow it. * 12- skip extraction, dir already exists. */

- /* do both a lstat and a stat, so we can see what symlinks point to */ struct stat lsbuf, sbuf; - if(_alpm_lstat(filename, &lsbuf) != 0 || stat(filename, &sbuf) != 0) { - /* cases 1,2,3: couldn't stat an existing file, skip all backup checks */ + if(stat(filename, &sbuf) != 0 || _alpm_lstat(filename, &lsbuf) != 0) { + /* cases 1,2,3: file doesn't exist or is a broken symlink, + * skip all backup checks */ + /* TODO: should we really treat broken symlinks as if they don't exist? */

NO! We definitely should not... In fact, I'd like to see this patch result in this: * (F=file, N=node, S=symlink, D=dir) * | F/N/S | D * non-existent | 1 | 2 * F/N/S | 3 | 4 * D | 5 | 6 because now there is no difference between a file and a symlink. Which probably means getting rid of the lstat call (although I did not look at the code to confirm...)

} else { if(S_ISDIR(lsbuf.st_mode)) { if(S_ISDIR(entrymode)) { @@ -243,23 +241,8 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive, archive_read_data_skip(archive); return 1; } - } else if(S_ISLNK(lsbuf.st_mode) && S_ISDIR(entrymode)) { - /* case 9: existing symlink, dir in package */ - if(S_ISDIR(sbuf.st_mode)) { - /* the symlink on FS is to a directory, so we'll use it */ - _alpm_log(handle, ALPM_LOG_DEBUG, "extract: skipping symlink overwrite of %s\n", - filename); - archive_read_data_skip(archive); - return 0; - } else { - /* this is BAD. symlink was not to a directory */ - _alpm_log(handle, ALPM_LOG_ERROR, _("extract: symlink %s does not point to dir\n"), - filename); - archive_read_data_skip(archive); - return 1; - } - } else if(S_ISREG(lsbuf.st_mode) && S_ISDIR(entrymode)) { - /* case 6: trying to overwrite file with dir */ + } else if(!S_ISDIR(lsbuf.st_mode) && S_ISDIR(entrymode)) { + /* case 6,9: trying to overwrite file with dir */ _alpm_log(handle, ALPM_LOG_DEBUG, "extract: overwriting file with dir %s\n", filename); } else if(S_ISREG(entrymode)) {