Project

General

Profile

Bug #781

overrun in fgetws

Added by Garrett D'Amore over 9 years ago. Updated over 9 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

History

#1

Updated by Garrett D'Amore over 9 years ago

  • Status changed from New to In Progress
#2

Updated by Garrett D'Amore over 9 years ago

  • Status changed from In Progress to Resolved

Fixed in:

changeset: 13299:ad3f52808747
tag: tip
user: Bryan Cantrill <>
date: Wed Mar 09 09:02:25 2011 -0800
description:
781 overrun in fgetws
Reviewed by: Garrett D'Amore <>
Reviewed by: Gordon Ross <>
Approved by: Garrett D'Amore <>

#3

Updated by Garrett D'Amore over 9 years ago

  • % Done changed from 30 to 100

Also available in: Atom PDF