On Wed, Aug 17, 2011 at 10:15:17AM +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 --- Another approach is to use a random default destfile name. Since the URL parsing is very unlikely to fail, I took the easy option here.

lib/libalpm/dload.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 731d807..c552d2b 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -373,6 +373,11 @@ static int curl_download_internal(struct dload_payload *payload, destfile = get_fullpath(localpath, effective_filename, ""); } } + else { + _alpm_log(handle, ALPM_LOG_ERROR, _("could not parse file name from url (%s)"), + effective_url); + goto cleanup; + } }

ret = 0; -- 1.7.6

This is after the file has already been downloaded. We shouldn't bail out so easily since we already have the file. It looks like in this case, we wouldn't even unlink after jumping to cleanup. I'm not sure that this is the right thing to do. Worst case scenario, we should probably just skip the rename. d

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 --- 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); } } -- 1.7.6

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 --- 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

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. Also, use the STRDUP macro instead of strdup() for the sake of better error handling on memory allocation failures. Signed-off-by: Lukas Fleischer --- 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..e281578 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) { + STRDUP(*final_file, strrchr(destfile, '/') + 1, RET_ERR(handle, ALPM_ERR_MEMORY, -1)); + } } else if(final_file) { - *final_file = strdup(strrchr(destfile, '/') + 1); + STRDUP(*final_file, strrchr(tempfile, '/') + 1, RET_ERR(handle, ALPM_ERR_MEMORY, -1)); } } -- 1.7.6

On Wed, Aug 17, 2011 at 03:05:55PM +0200, Lukas Fleischer wrote:

On Wed, Aug 17, 2011 at 08:37:23AM -0400, Dave Reisner wrote:

...

On Wed, Aug 17, 2011 at 10:15:16AM +0200, Lukas Fleischer wrote:

...

stat()'s behaviour is undefined if the first argument is NULL and might be prone to segfault. Add an additional check to skip the stat() invocation if no destfile is used.

Signed-off-by: Lukas Fleischer --- lib/libalpm/dload.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 5a63e48..731d807 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -260,7 +260,7 @@ static int curl_download_internal(struct dload_payload *payload, curl_easy_setopt(handle->curl, CURLOPT_USERAGENT, useragent); }

- if(!payload->allow_resume && !payload->force && stat(destfile, &st) == 0) { + if(!payload->allow_resume && !payload->force && destfile && stat(destfile, &st) == 0) { /* start from scratch, but only download if our local is out of date. */ curl_easy_setopt(handle->curl, CURLOPT_TIMECONDITION, CURL_TIMECOND_IFMODSINCE); curl_easy_setopt(handle->curl, CURLOPT_TIMEVALUE, (long)st.st_mtime); -- 1.7.6

We already check for destfile being NULL earlier, when we try to create it, line 210ish.

Yeah, we do not check that in the else branch though.

And this is what I get for looking at this on the wrong branch. Yeah, this makes sense.