Project

General

Profile

Actions

Bug #781

closed

overrun in fgetws

Added by Garrett D'Amore about 11 years ago. Updated about 11 years ago.

Status:
Resolved
Priority:
Normal
Category:
lib - userland libraries
Start date:
2011-03-08
Due date:
% Done:

100%

Estimated time:
1.00 h
Difficulty:
Tags:
Gerrit CR:

Description

This from Bryan Cantrill:

In debugging another problem, sort(1) dumped core on me. (Both the
file that induced the core dump and the core dump itself are available
-- for a limited time -- at ftp://dapsays.no.de.)

Here's the stack trace in the dump:

> $c
libc_hwcap1.so.1`_fgetws_impl+0xa3(fe936ec4, 4f, 80757b0, 1, 8047c38)
libc_hwcap1.so.1`__fgetws_xpg5+0x23(fe936ec4, 4f, 80757b0, 808de80)
stream_wide_fetch+0x63(808de80, fe0384d0, 2, 0)
stream_insert+0xc0(806d380, 808de80, 808ddd0, 0)
internal_sort+0x18f(806d380, 3, 8047d14, feffbafc)
main+0x70(3, 8047d14, 8047d24, 805384f)
_start+0x7d(3, 8047de4, 8047df6, 8047df9, 0, 8047e00)

Died here:

> libc_hwcap1.so.1`_fgetws_impl+0xa3/i
libc_hwcap1.so.1`_fgetws_impl+0xa3: movl %eax,(%edi)
> <edi=X
fe937000
> <edi/X
mdb: failed to read data from target: no mapping for address
0xfe937000:

Suspiciously, %edi is exactly the input pointer (0xfe936ec4) plus the
number of bytes (79) times the sizeof of a wchar_t (4). It seems that
_fgetws_impl() might have an off-by-one bug, and indeed, from the code
it seems that -- in the case that we read to the end of the buffer --
we overwrite the buffer by one byte. The attached (excruciatingly
simple) diff fixes the problem for me (dead reproducible on the
input), but others may want to confirm that it's the proper fix...

- Bryan

His fix:

f --git a/usr/src/lib/libc/port/locale/fgetws.c b/usr/src/lib/libc/port/locale/fgetws.c index e209023..ba2f60e 100644 --- a/usr/src/lib/libc/port/locale/fgetws.c +++ b/usr/src/lib/libc/port/locale/fgetws.c @@ -63,7 +63,7 @@ _fgetws_impl(wchar_t *_RESTRICT_KYWD ws, int n, FILE *_RESTRICT_KYWD fp, } wsp = ws; - while (n--) { + while (n-- > 1) { wc = _fgetwc_unlocked(fp); if (wc == EOF) { /*

And my refinement:

This is probably a bug I introduced, as I rewrote most of that function
from the original FreeBSD code that I imported. :-/

Your fix looks good, but I think I would prefer to see it as:

while (--n) {

This should also be slightly more efficient, since it can then generate
to simple branch-if-zero following the decrement, avoiding the extra
compare (subtract) against 1.

Its safe too, because the previous stanza guards against the case where
n == 0.

If you like my version of the fix, let me know, and I'll go ahead and
start the process to get it integrated.

- Garrett
Actions

Also available in: Atom PDF