[pacman-dev] [PATCH] Reset dload_interrupted early in curl_download_internal()

[Lukas Fleischer]

On Thu, Aug 18, 2011 at 07:37:24AM -0500, Dan McGee wrote:
> On Thu, Aug 18, 2011 at 2:13 AM, Lukas Fleischer
> <archlinux at cryptocrack.de> 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 at 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
> 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.