[pacman-dev] [PATCH 1/2] Avoid stat() on NULL path in curl_download_internal()
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 <archlinux@cryptocrack.de> --- 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
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> --- 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
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 <archlinux@cryptocrack.de> --- 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
On Wed, Aug 17, 2011 at 09:40:22AM -0400, Dave Reisner wrote:
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 <archlinux@cryptocrack.de> --- 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.
Oh, right. I missed that part.
I'm not sure that this is the right thing to do. Worst case scenario, we should probably just skip the rename.
Yeah, either use some default for destfile (some random name or just set "destfile = strdup(tempfile);") or skip the rename (I prefer the latter).
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); } } -- 1.7.6
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.
} }
-- 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 <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
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.
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 <archlinux@cryptocrack.de> --- 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 Thu, Aug 18, 2011 at 07:50:06AM +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.
Also, use the STRDUP macro instead of strdup() for the sake of better error handling on memory allocation failures.
Signed-off-by: Lukas Fleischer <archlinux@cryptocrack.de> --- lib/libalpm/dload.c | 14 +++++++++----- 1 files changed, 9 insertions(+), 5 deletions(-)
Crap, just noticed that Dan already pushed this one...
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 <archlinux@cryptocrack.de> --- 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. d
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 <archlinux@cryptocrack.de> --- 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.
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 <archlinux@cryptocrack.de> --- 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.
participants (2)
-
Dave Reisner
-
Lukas Fleischer