[PATCH] wget: removed pointless free, fixes bug accessing freed memory

[Denys Vlasenko]

On Tuesday 15 March 2011 03:07, gotrunks at gmail.com wrote:
> Hi,
> 
> I have just noticed a recent wget bug. Busybox wget doesn't parse the
> URL correctly in some circumstances; long guessed output filenames &&
> HTTP redirection.
> 
> e.g.
> $./busybox wget
> http://cdimage.debian.org/debian-cd/6.0.0/kfreebsd-i386/iso-cd/debian-6.0.0-kfreebsd-i386-CD-1.iso
> Connecting to cdimage.debian.org (130.239.18.173:80)
> Connecting to hammurabi.acc.umu.se (130.239.18.165:80)
> wget: can't open '': No such file or directory

Can't reproduce. What causes this on your machine?


> and AFAIS it's pointless to free(h->allocated) on parse_url(), because
> we already free it at the end (and other places) of
> download_one_url().

There is a case when parse_url(str, &target) is called repeatedly
if we traverse a long chain of redirects:


 resolve_lsa:
        lsa = xhost2sockaddr(server.host, server.port);
        if (!(option_mask32 & WGET_OPT_QUIET)) {
                char *s = xmalloc_sockaddr2dotted(&lsa->u.sa);
                fprintf(stderr, "Connecting to %s (%s)\n", server.host, s);
                free(s);
        }
 establish_session:
        /*G.content_len = 0; - redundant, got_clen = 0 is enough */
        G.got_clen = 0;
        G.chunked = 0;
        if (use_proxy || !target.is_ftp) {
...
                /*
                 * Retrieve HTTP headers.
                 */
                while ((str = gethdr(sfp)) != NULL) {
...
                        if (key == KEY_location && status >= 300) {
                                if (--redir_limit == 0)
                                        bb_error_msg_and_die("too many redirections");
                                fclose(sfp);
                                if (str[0] == '/') {
                                        free(target.allocated);
                                        target.path = target.allocated = xstrdup(str+1);
                                        /* lsa stays the same: it's on the same server */
                                } else {
HERE ===>                               parse_url(str, &target);
                                        if (!use_proxy) {
                                                free(server.allocated);
                                                server.allocated = NULL;
                                                server.host = target.host;
                                                /* strip_ipv6_scope_id(target.host); - no! */
                                                /* we assume remote never gives us IPv6 addr with scope id */
                                                server.port = target.port;
                                                free(lsa);
                                                goto resolve_lsa;
                                        } /* else: lsa stays the same: we use proxy */
                                }
                                goto establish_session;
                        }

I don't want to leak target.allocated in this case.
-- 
vda