Date: Tue, 24 Aug 2004 20:08:12 +0200 (CEST) From: Harti Brandt <harti@freebsd.org> To: Sean McNeil <sean@mcneil.com> Cc: current@freebsd.org Subject: Re: bsdtar core dumps Message-ID: <20040824200225.V517@beagle.kn.op.dlr.de> In-Reply-To: <1093369220.10362.6.camel@server.mcneil.com> References: <1092777586.92327.9.camel@server.mcneil.com> <20040817213813.GE3827@gothmog.gr><4127841D.6050104@freebsd.org> <1093369220.10362.6.camel@server.mcneil.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 24 Aug 2004, Sean McNeil wrote:
SM>On Sat, 2004-08-21 at 10:19, Tim Kientzle wrote:
SM>> Sean McNeil wrote:
SM>> >>>
SM>> >>>I just tried to unarchive a file that didn't exist and got a core dump:
SM>> >
SM>> > Here is a backtrace of the error:
SM>> >
SM>> > #0 0x0000000200926d7e in __vfprintf (fp=0x7fffffffe360,
SM>> > fmt0=0x4161d9 "Failed to open '%s'", ap=0x7fffffffe640)
SM>> > at /usr/src/lib/libc/stdio/vfprintf.c:1052
SM>> > #1 0x00000002008c4006 in vsnprintf (str=0x32 <Address 0x32 out of bounds>,
SM>> > n=4284889, fmt=0x4161d9 "Failed to open '%s'", ap=0x7fffffffe640)
SM>> > at /usr/src/lib/libc/stdio/vsnprintf.c:75
SM>> > #2 0x0000000000411478 in __archive_string_vsprintf (as=0x520240,
SM>> > fmt=0x4161d9 "Failed to open '%s'", ap=0x7fffffffe640)
SM>> > at /usr/src/lib/libarchive/archive_string_sprintf.c:60
SM>> >
SM>> > Could be a compiler bug I suppose, but more likely I think it is this
SM>> > code:
SM>> >
SM>> > if (n == 0) {
SM>> > if (on > 0)
SM>> > *str = '\0';
SM>> > str = dummy;
SM>> > n = 1;
SM>> > }
SM>> >
SM>> > in vsnprintf.c::vsnprintf.
SM>>
SM>> The code you've pointed to above concerns
SM>> me because of the part about:
SM>> if (n == 0) {
SM>> ...
SM>> n = 1;
SM>> }
SM>> That ain't right: If I told vsnprintf the buffer
SM>> size was zero, it should treat it as such. If I
SM>> meant "one", I would have said "one."
SM>>
SM>> On the other hand, the vsnprintf.3 man page
SM>> does explicitly state that "the output is always
SM>> null-terminated," which would preclude passing
SM>> a zero-length buffer, which is exactly what
SM>> libarchive is doing in this situation. It is
SM>> bogus, but at least it's documented bogosity. ;-)
SM>>
SM>> Please try the attached patch to libarchive/archive_string_sprintf.c
SM>> and let me know if it works for you. It simply
SM>> forces the target buffer to be allocated and thereby
SM>> avoids calling vsnprintf with a NULL buffer.
SM>>
SM>> Tim Kientzle
SM>>
SM>> ______________________________________________________________________
SM>> Index: archive_string_sprintf.c
SM>> ===================================================================
SM>> RCS file: /home/ncvs/src/lib/libarchive/archive_string_sprintf.c,v
SM>> retrieving revision 1.4
SM>> diff -u -r1.4 archive_string_sprintf.c
SM>> --- archive_string_sprintf.c 14 Aug 2004 03:45:45 -0000 1.4
SM>> +++ archive_string_sprintf.c 21 Aug 2004 17:02:49 -0000
SM>> @@ -48,6 +48,9 @@
SM>> {
SM>> size_t l;
SM>>
SM>> + /* Make sure the target area is initialized. */
SM>> + __archive_string_ensure(as, 64);
SM>> +
SM>> if (fmt == NULL) {
SM>> as->s[0] = 0;
SM>> return;
SM>
SM>I think what is happening is that the amd64 architecture passes a
SM>va_list by reference instead of by value. This is causing a side-effect
SM>within __vfprintf. To counter the side-effect, the following patch
SM>saves the ap and uses the copy for the second call to vsnprintf. Here
SM>is a patch that fixes my core dump:
Sorry to jump in.
You cannot use a va_list twice. As soon as someone call va_arg() on the
ap all the aps in the calling functions get invalid. The only thing that
can and must be done is that the function that did the va_start() must
call va_end.
If you need it twice you must make a copy as in the patch below.
But the function call va_copy must also call va_end() on that copy
(this seems missing in the patch).
harti
SM>
SM>*** lib/libarchive/archive_string_sprintf.c.orig Fri Aug 13
SM>20:45:45 2004--- lib/libarchive/archive_string_sprintf.c Tue Aug 24
SM>10:37:02 2004
SM>*************** __archive_string_vsprintf(struct archive
SM>*** 47,63 ****
SM> va_list ap)
SM> {
SM> size_t l;
SM>
SM> if (fmt == NULL) {
SM> as->s[0] = 0;
SM> return;
SM> }
SM>
SM> l = vsnprintf(as->s, as->buffer_length, fmt, ap);
SM> /* If output is bigger than the buffer, resize and try again. */
SM> if (l+1 >= as->buffer_length) {
SM> __archive_string_ensure(as, l + 1);
SM>! l = vsnprintf(as->s, as->buffer_length, fmt, ap);
SM> }
SM> as->length = l;
SM> }
SM>--- 47,65 ----
SM> va_list ap)
SM> {
SM> size_t l;
SM>+ va_list ap1;
SM>
SM> if (fmt == NULL) {
SM> as->s[0] = 0;
SM> return;
SM> }
SM>
SM>+ va_copy(ap1,ap);
SM> l = vsnprintf(as->s, as->buffer_length, fmt, ap);
SM> /* If output is bigger than the buffer, resize and try again. */
SM> if (l+1 >= as->buffer_length) {
SM> __archive_string_ensure(as, l + 1);
SM>! l = vsnprintf(as->s, as->buffer_length, fmt, ap1);
SM> }
SM> as->length = l;
SM> }
SM>
SM>
SM>_______________________________________________
SM>freebsd-current@freebsd.org mailing list
SM>http://lists.freebsd.org/mailman/listinfo/freebsd-current
SM>To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org"
SM>
SM>
SM>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040824200225.V517>