[pacman-dev] [PATCH] Reset dload_interrupted early in curl_download_internal()
Avoid invoking unlink() with a NULL path if the URL isn't to a file and we fail to create a temporary file. Signed-off-by: Lukas Fleischer <archlinux@cryptocrack.de> --- lib/libalpm/dload.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 5cbb6b4..050e719 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -195,6 +195,9 @@ static int curl_download_internal(struct dload_payload *payload, /* shortcut to our handle within the payload */ alpm_handle_t *handle = payload->handle; + /* reset dload_interrupted early to avoid erroneous unlink() on cleanup */ + dload_interrupted = 0; + if(!payload->filename) { payload->filename = get_filename(payload->fileurl); } @@ -289,7 +292,6 @@ static int curl_download_internal(struct dload_payload *payload, sigaction(SIGPIPE, NULL, &sig_pipe[OLD]); sigaction(SIGPIPE, &sig_pipe[NEW], NULL); - dload_interrupted = 0; sig_int[NEW].sa_handler = &inthandler; sigemptyset(&sig_int[NEW].sa_mask); sig_int[NEW].sa_flags = 0; -- 1.7.6
On Wed, Aug 17, 2011 at 04:54:56PM +0200, Lukas Fleischer wrote:
Avoid invoking unlink() with a NULL path if the URL isn't to a file and we fail to create a temporary file.
Signed-off-by: Lukas Fleischer <archlinux@cryptocrack.de> --- lib/libalpm/dload.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
Note that this still isn't fixed with the latest refactoring (not sure if 9f139550 was supposed to fix this). If we fail to create a temporary file (around line 224), tempfile still points to NULL and the unlink() invocation might result in a segfault.
On Wed, Aug 17, 2011 at 04:54:56PM +0200, Lukas Fleischer wrote:
Avoid invoking unlink() with a NULL path if the URL isn't to a file and we fail to create a temporary file.
Signed-off-by: Lukas Fleischer <archlinux@cryptocrack.de> --- lib/libalpm/dload.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
Note that this still isn't fixed with the latest refactoring (not sure if 9f139550 was supposed to fix this). If we fail to create a temporary file (around line 224), tempfile still points to NULL and the unlink() invocation might result in a segfault. Can you outline *exactly* what needs to happen for this case to take
On Thu, Aug 18, 2011 at 2:13 AM, Lukas Fleischer <archlinux@cryptocrack.de> wrote: place? I'm not seeing it, or at least I don't think your case is when it would happen. If the STRDUP() into tempfile fails, we return -1 on the spot and don't continue any further. If on the other hand, ret stays at its default -1, payload->allow_resume is 0 (making should_unlink 1), and we goto cleanup before tempfile has been allocated, this could be a problem. Hoewver, I see no code path where this is possible. -Dan
On Thu, Aug 18, 2011 at 07:37:24AM -0500, Dan McGee wrote:
On Wed, Aug 17, 2011 at 04:54:56PM +0200, Lukas Fleischer wrote:
Avoid invoking unlink() with a NULL path if the URL isn't to a file and we fail to create a temporary file.
Signed-off-by: Lukas Fleischer <archlinux@cryptocrack.de> --- lib/libalpm/dload.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
Note that this still isn't fixed with the latest refactoring (not sure if 9f139550 was supposed to fix this). If we fail to create a temporary file (around line 224), tempfile still points to NULL and the unlink() invocation might result in a segfault. Can you outline *exactly* what needs to happen for this case to take
On Thu, Aug 18, 2011 at 2:13 AM, Lukas Fleischer <archlinux@cryptocrack.de> wrote: place? I'm not seeing it, or at least I don't think your case is when it would happen. If the STRDUP() into tempfile fails, we return -1 on the spot and don't continue any further.
If on the other hand, ret stays at its default -1, payload->allow_resume is 0 (making should_unlink 1), and we goto cleanup before tempfile has been allocated, this could be a problem. Hoewver, I see no code path where this is possible.
Yes, this might happen if we enter the else branch in line 214 ("dload.c", current HEAD is 9f139550) and either mkstemp() or fdopen() fail in line 224. In that case, randpath is unlinked, the file descriptor opened by mkstemp() is closed and we jump to the cleanup label. Note that at this point, tempfile is still NULL, while ret is set to -1, so "((ret == -1 || dload_interrupted) && should_unlink)" evaluates to true and unlink() is invoked with a NULL pointer.
participants (2)
-
Dan McGee
-
Lukas Fleischer