Re: bsdtar core dumps

On Tue, 2004-08-24 at 13:03, Sean McNeil wrote:
> On Tue, 2004-08-24 at 12:03, Harti Brandt wrote:
> > On Tue, 24 Aug 2004, Sean McNeil wrote:
> > 
> > SM>On Tue, 2004-08-24 at 11:08, Harti Brandt wrote:
> > SM>> On Tue, 24 Aug 2004, Sean McNeil wrote:
> > SM>> 
> > SM>[snip]
> > SM>> 
> > SM>> Sorry to jump in.
> > SM>> 
> > SM>> You cannot use a va_list twice. As soon as someone call va_arg() on the
> > SM>> ap all the aps in the calling functions get invalid. The only thing that 
> > SM>> can and must be done is that the function that did the va_start() must 
> > SM>> call va_end.
> > SM>> 
> > SM>> If you need it twice you must make a copy as in the patch below.
> > SM>> But the function call va_copy must also call va_end() on that copy
> > SM>> (this seems missing in the patch).
> > SM>
> > SM>I took the lead from /usr/src/lib/libc/stdio/vfprintf.c
> > SM>
> > SM>It happens to do a va_copy(orgap,ap) but never does a va_end(orgap). 
> > SM>Either vfprintf.c is incorrect or it is not necessary to call va_end().
> > 
> > vfprintf.c is wrong. Paragraph 1 of 7.15.1 of C99 states: "Each invocation 
> > of the va_start and va_copy macros shall be matched by a corresponding 
> > invocation of the va_end macro in the same function."
> 
> C99 is good enough for me.  I suppose this is so that va_copy can be
> implemented with some sort of allocation scheme.  Could someone commit
> fixes to vfprintf.c and archive_string_sprintf.c?
> 
> *** lib/libarchive/archive_string_sprintf.c.orig        Fri Aug 13
> 20:45:45 2004--- lib/libarchive/archive_string_sprintf.c     Tue Aug 24
> 12:58:24 2004
> *************** __archive_string_vsprintf(struct archive
> *** 47,65 ****
>       va_list ap)
>   {
>         size_t l;
> 
>         if (fmt == NULL) {
>                 as->s[0] = 0;
>                 return;
>         }
> 
>         l = vsnprintf(as->s, as->buffer_length, fmt, ap);
>         /* If output is bigger than the buffer, resize and try again. */
>         if (l+1 >= as->buffer_length) {
>                 __archive_string_ensure(as, l + 1);
> !               l = vsnprintf(as->s, as->buffer_length, fmt, ap);
>         }
>         as->length = l;
>   }
> 
>   /*
> --- 47,68 ----
>       va_list ap)
>   {
>         size_t l;
> +       va_list ap1;
> 
>         if (fmt == NULL) {
>                 as->s[0] = 0;
>                 return;
>         }
> 
> +       va_copy(ap1,ap);
>         l = vsnprintf(as->s, as->buffer_length, fmt, ap);
>         /* If output is bigger than the buffer, resize and try again. */
>         if (l+1 >= as->buffer_length) {
>                 __archive_string_ensure(as, l + 1);
> !               l = vsnprintf(as->s, as->buffer_length, fmt, ap1);
>         }
>         as->length = l;
> +       va_end(ap1);
>   }
> 
>   /*
> 
> *** lib/libc/stdio/vfprintf.c.orig      Mon Aug 23 20:35:43 2004
> --- lib/libc/stdio/vfprintf.c   Tue Aug 24 13:01:45 2004
> *************** error:
> *** 1237,1242 ****
> --- 1237,1243 ----
>         if (dtoaresult != NULL)
>                 freedtoa(dtoaresult);
>   #endif
> +       va_end(orgap);
>         if (convbuf != NULL)
>                 free(convbuf);
>         if (__sferror(fp))

I missed one in vfwprintf.c.  I needs a va_end(orgap) just like above.

Also, I've searched all of src and found one additional place that there
is a va_copy without a va_end.

contrib/gnu-sort/lib/version-etc.c

All other uses appear to be properly matched.

Cheers,
Sean