On Wed, Aug 17, 2011 at 11:45:35AM -0400, Dave Reisner wrote:
On Wed, Aug 17, 2011 at 11:34:19AM -0400, Dave Reisner wrote:
On Wed, Aug 17, 2011 at 04:45:35PM +0200, Lukas Fleischer wrote:
Avoid a potential segfault that may occur if we use a temporary file and fail to build the destination file name from the effective URL.
Signed-off-by: Lukas Fleischer <archlinux@cryptocrack.de> --- Untested, sorry. Should be trivial enough not to break anything though. Hopefully.
lib/libalpm/dload.c | 14 +++++++++----- 1 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 731d807..5cbb6b4 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -384,12 +384,16 @@ cleanup: }
if(ret == 0) { - if(rename(tempfile, destfile)) { - _alpm_log(handle, ALPM_LOG_ERROR, _("could not rename %s to %s (%s)\n"), - tempfile, destfile, strerror(errno)); - ret = -1; + if (destfile) { + if(rename(tempfile, destfile)) { + _alpm_log(handle, ALPM_LOG_ERROR, _("could not rename %s to %s (%s)\n"), + tempfile, destfile, strerror(errno)); + ret = -1; + } else if(final_file) { + *final_file = strdup(strrchr(destfile, '/') + 1); + } } else if(final_file) { - *final_file = strdup(strrchr(destfile, '/') + 1); + *final_file = strdup(strrchr(tempfile, '/') + 1);
If you're going to change this line, can you use the STRDUP macro like I should have?
STRDUP(final_file, tempfile, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
Same for the added strdup inside the if(destfile) block. Looks fine otherwise.
} }
Note that this doesn't actually solve the issue of a zero length filename...
┌┤noclaf@rampage:1 bugfix ~/src/c/pacman └╼ sudo ./src/pacman/pacman -U http://www.archlinux.org/ alpmtmp.sj9IKg 23.2K 443.7K/s 00:00:00 [-----------------------] 100% error: could not rename /mnt/Haven/packages/alpmtmp.sj9IKg to /mnt/Haven/packages/ (Not a directory) warning: failed to download http://www.archlinux.org/ error: 'http://www.archlinux.org/': unexpected error
Well, this is right and I would say we shouldn't even try to fix this here. There might be other file system dependent limitations to file names. If you want to do this (kind of) properly, there should be another patch that checks for the length of the file name, as well as a set of allowed characters (intersection of what the common file systems accept) and replace everything else. The intention of this patch is to fix a potential segfault by ensuring that we never rename() and/or strrchr() a NULL pointer.