Fix for heap overflow in wscanf (BZ 16618)
diff mbox

Message ID CALoOobNFbi8csanuAGDwebQeojNWsSqj+6g6w-J94hZ8POOZiw@mail.gmail.com
State New
Headers show

Commit Message

Paul Pluzhnikov Feb. 2, 2015, 6:59 p.m. UTC
On Mon, Feb 2, 2015 at 10:45 AM, Andreas Schwab <schwab@suse.de> wrote:
                 \
>> +               __set_errno (ENOMEM);                                     \
>
> You already have a meaningful errno from the failed realloc.

I see. And we are sure that "free(old)" will not touch errno?

Patch updated.

Comments

Carlos O'Donell Feb. 3, 2015, 4:24 p.m. UTC | #1
On 02/02/2015 01:59 PM, Paul Pluzhnikov wrote:
> On Mon, Feb 2, 2015 at 10:45 AM, Andreas Schwab <schwab@suse.de> wrote:
>                  \
>>> +               __set_errno (ENOMEM);                                     \
>>
>> You already have a meaningful errno from the failed realloc.
> 
> I see. And we are sure that "free(old)" will not touch errno?

Yes, free can never fail. If free were to touch errno it would be a bug,
or would only happen in cases of undefined behaviour.

Cheers,
Carlos.
Rich Felker Feb. 3, 2015, 6:01 p.m. UTC | #2
On Tue, Feb 03, 2015 at 11:24:08AM -0500, Carlos O'Donell wrote:
> On 02/02/2015 01:59 PM, Paul Pluzhnikov wrote:
> > On Mon, Feb 2, 2015 at 10:45 AM, Andreas Schwab <schwab@suse.de> wrote:
> >                  \
> >>> +               __set_errno (ENOMEM);                                     \
> >>
> >> You already have a meaningful errno from the failed realloc.
> > 
> > I see. And we are sure that "free(old)" will not touch errno?
> 
> Yes, free can never fail. If free were to touch errno it would be a bug,
> or would only happen in cases of undefined behaviour.

free can make a syscall that will set errno unless you suppress this
behavior -- munmap can fail due to inability to split an existing vma
due to hitting the vma limit or simply a kernel oom condition. In this
case I suspect you get the result you want, ENOMEM, anyway, but in
general if you want free to have a contract not to set errno, that
requires some attention, and it's almost surely not satisfied if
applications are allowed to replace free...

Rich
Carlos O'Donell Feb. 3, 2015, 6:06 p.m. UTC | #3
On 02/03/2015 01:01 PM, Rich Felker wrote:
> On Tue, Feb 03, 2015 at 11:24:08AM -0500, Carlos O'Donell wrote:
>> On 02/02/2015 01:59 PM, Paul Pluzhnikov wrote:
>>> On Mon, Feb 2, 2015 at 10:45 AM, Andreas Schwab <schwab@suse.de> wrote:
>>>                  \
>>>>> +               __set_errno (ENOMEM);                                     \
>>>>
>>>> You already have a meaningful errno from the failed realloc.
>>>
>>> I see. And we are sure that "free(old)" will not touch errno?
>>
>> Yes, free can never fail. If free were to touch errno it would be a bug,
>> or would only happen in cases of undefined behaviour.
> 
> free can make a syscall that will set errno unless you suppress this
> behavior -- munmap can fail due to inability to split an existing vma
> due to hitting the vma limit or simply a kernel oom condition. In this
> case I suspect you get the result you want, ENOMEM, anyway, but in
> general if you want free to have a contract not to set errno, that
> requires some attention, and it's almost surely not satisfied if
> applications are allowed to replace free...

I would consider all of these things bugs and particularly in the use
case of the user-provide free() a conformance issue for modifying errno
without there being an error.

Cheers,
Carlos.
Rich Felker Feb. 3, 2015, 6:41 p.m. UTC | #4
On Tue, Feb 03, 2015 at 01:06:14PM -0500, Carlos O'Donell wrote:
> On 02/03/2015 01:01 PM, Rich Felker wrote:
> > On Tue, Feb 03, 2015 at 11:24:08AM -0500, Carlos O'Donell wrote:
> >> On 02/02/2015 01:59 PM, Paul Pluzhnikov wrote:
> >>> On Mon, Feb 2, 2015 at 10:45 AM, Andreas Schwab <schwab@suse.de> wrote:
> >>>                  \
> >>>>> +               __set_errno (ENOMEM);                                     \
> >>>>
> >>>> You already have a meaningful errno from the failed realloc.
> >>>
> >>> I see. And we are sure that "free(old)" will not touch errno?
> >>
> >> Yes, free can never fail. If free were to touch errno it would be a bug,
> >> or would only happen in cases of undefined behaviour.
> > 
> > free can make a syscall that will set errno unless you suppress this
> > behavior -- munmap can fail due to inability to split an existing vma
> > due to hitting the vma limit or simply a kernel oom condition. In this
> > case I suspect you get the result you want, ENOMEM, anyway, but in
> > general if you want free to have a contract not to set errno, that
> > requires some attention, and it's almost surely not satisfied if
> > applications are allowed to replace free...
> 
> I would consider all of these things bugs and particularly in the use
> case of the user-provide free() a conformance issue for modifying errno
> without there being an error.

ISO C and POSIX explicitly permit any function to modify errno on
success as long as they don't set it to zero. free always succeeds, so
it's always valid for free to clobber errno. This is not a conformance
issue. If you assume free does not modify errno, that's a non-portable
assumption from the application side and a flawed assumption from the
glibc side assuming you allow replacement of malloc/free.

Rich
Carlos O'Donell Feb. 3, 2015, 7:22 p.m. UTC | #5
On 02/03/2015 01:41 PM, Rich Felker wrote:
>> I would consider all of these things bugs and particularly in the use
>> case of the user-provide free() a conformance issue for modifying errno
>> without there being an error.
> 
> ISO C and POSIX explicitly permit any function to modify errno on
> success as long as they don't set it to zero. free always succeeds, so
> it's always valid for free to clobber errno. This is not a conformance
> issue. If you assume free does not modify errno, that's a non-portable
> assumption from the application side and a flawed assumption from the
> glibc side assuming you allow replacement of malloc/free.

I had not interpreted the wording as you paraphrase it, but I can see
how it could be interpreted like that.

My expectation had simply been that functions that don't define any
conditions under which errno is defined, would by this fact not have
any need to modify errno.

Thus without a need to modify errno I conflated that to mean "should
not modify errno," but the truth is they can actually do the opposite
and have free reign to change the value at will (except 0).

So we have:
* free() doesn't say it sets errno, therefore errno is undefined until
  it is changed by the next function call or application code
* errno after success is unspecified because the description of free()
  does not say anything about errno on success
* errno can't be examined because free() never describes when it might
  be valid to do so
* errno is not set to 0 after a successful call to free()

So free() is, under the standard, free to write any value to errno 
except 0, and it is the application that may not examine errno.

Thanks, I'd read the POSIX wording differently.

Cheers,
Carlos.
Paul Eggert Feb. 4, 2015, 12:11 a.m. UTC | #6
Carlos O'Donell wrote:
> I'd read the POSIX wording differently.

Although Rich's interpretation is correct for current POSIX, thanks to Eric 
Blake the next release of POSIX (Issue 8) is planned to change this, and to 
require 'free' to leave errno alone, which as I understand it is your preferred 
interpretation.  Please see:

http://austingroupbugs.net/view.php?id=385

Because of this, glibc 'free' should not set errno if the user invokes 'free' in 
a conforming way.  Setting errno will be a conformance bug once Issue 8 comes 
out, and glibc should be fixed to conform well before that.  Also, the glibc 
documentation should be changed to discuss this issue.  I have filed a glibc bug 
report to that effect, here:

https://sourceware.org/bugzilla/show_bug.cgi?id=17924
Rich Felker Feb. 4, 2015, 12:27 a.m. UTC | #7
On Tue, Feb 03, 2015 at 04:11:56PM -0800, Paul Eggert wrote:
> Carlos O'Donell wrote:
> >I'd read the POSIX wording differently.
> 
> Although Rich's interpretation is correct for current POSIX, thanks
> to Eric Blake the next release of POSIX (Issue 8) is planned to
> change this, and to require 'free' to leave errno alone, which as I
> understand it is your preferred interpretation.  Please see:
> 
> http://austingroupbugs.net/view.php?id=385
> 
> Because of this, glibc 'free' should not set errno if the user
> invokes 'free' in a conforming way.  Setting errno will be a
> conformance bug once Issue 8 comes out, and glibc should be fixed to
> conform well before that.  Also, the glibc documentation should be
> changed to discuss this issue.  I have filed a glibc bug report to
> that effect, here:
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=17924

Interesting. Unfortunately this makes it impossible for the
application to observe the "valid memory was unable to be freed"
condition that occurs when you can't split a vma. Formally, the memory
is still freed anyway, so it hardly matters, but it indicates a
critical situation where things are about to blow up for the
application (malloc no longer working, etc.) so conceivably an
application could want to detect and respond to the condition.

Rich
Carlos O'Donell Feb. 4, 2015, 4:33 a.m. UTC | #8
On 02/03/2015 07:27 PM, Rich Felker wrote:
> On Tue, Feb 03, 2015 at 04:11:56PM -0800, Paul Eggert wrote:
>> Carlos O'Donell wrote:
>>> I'd read the POSIX wording differently.
>>
>> Although Rich's interpretation is correct for current POSIX, thanks
>> to Eric Blake the next release of POSIX (Issue 8) is planned to
>> change this, and to require 'free' to leave errno alone, which as I
>> understand it is your preferred interpretation.  Please see:
>>
>> http://austingroupbugs.net/view.php?id=385
>>
>> Because of this, glibc 'free' should not set errno if the user
>> invokes 'free' in a conforming way.  Setting errno will be a
>> conformance bug once Issue 8 comes out, and glibc should be fixed to
>> conform well before that.  Also, the glibc documentation should be
>> changed to discuss this issue.  I have filed a glibc bug report to
>> that effect, here:
>>
>> https://sourceware.org/bugzilla/show_bug.cgi?id=17924
> 
> Interesting. Unfortunately this makes it impossible for the
> application to observe the "valid memory was unable to be freed"
> condition that occurs when you can't split a vma. Formally, the memory
> is still freed anyway, so it hardly matters, but it indicates a
> critical situation where things are about to blow up for the
> application (malloc no longer working, etc.) so conceivably an
> application could want to detect and respond to the condition.

Could you expand a bit on the split vma issue? I'm not familiar
with that failure mode.

Cheers,
Carlos.
Rich Felker Feb. 4, 2015, 4:46 a.m. UTC | #9
On Tue, Feb 03, 2015 at 11:33:33PM -0500, Carlos O'Donell wrote:
> On 02/03/2015 07:27 PM, Rich Felker wrote:
> > On Tue, Feb 03, 2015 at 04:11:56PM -0800, Paul Eggert wrote:
> >> Carlos O'Donell wrote:
> >>> I'd read the POSIX wording differently.
> >>
> >> Although Rich's interpretation is correct for current POSIX, thanks
> >> to Eric Blake the next release of POSIX (Issue 8) is planned to
> >> change this, and to require 'free' to leave errno alone, which as I
> >> understand it is your preferred interpretation.  Please see:
> >>
> >> http://austingroupbugs.net/view.php?id=385
> >>
> >> Because of this, glibc 'free' should not set errno if the user
> >> invokes 'free' in a conforming way.  Setting errno will be a
> >> conformance bug once Issue 8 comes out, and glibc should be fixed to
> >> conform well before that.  Also, the glibc documentation should be
> >> changed to discuss this issue.  I have filed a glibc bug report to
> >> that effect, here:
> >>
> >> https://sourceware.org/bugzilla/show_bug.cgi?id=17924
> > 
> > Interesting. Unfortunately this makes it impossible for the
> > application to observe the "valid memory was unable to be freed"
> > condition that occurs when you can't split a vma. Formally, the memory
> > is still freed anyway, so it hardly matters, but it indicates a
> > critical situation where things are about to blow up for the
> > application (malloc no longer working, etc.) so conceivably an
> > application could want to detect and respond to the condition.
> 
> Could you expand a bit on the split vma issue? I'm not familiar
> with that failure mode.

Sure. When malloc uses mmap to allocate large chunks, they usually end
up immediately adjacent and merged into a single vma (virtual memory
area). If each were separated by gaps, you'd hit the kernel's vma
limit per process (default: 64k vmas) very quickly. Some hardened
kernels might aim to keep gaps, but in general this is very expensive
and not done in the mainline kernel.

Anyway, if you have a mmap-obtained chunk X with other anonymous
private mmaps (from malloc or otherwise) directly adjacent to it on
both sides, they're initially a single vma. But when free calls
munmap, the vma has to be split into three parts: the middle one
comprising X that's about to be destroyed, and new vmas for the
now-discontiguous parts left on each side. The result is a net
adjustment of +1 to the nummber of vmas, which may exceed the kernel
limit. In that case munmap will fail. I believe this case actually
used to crash glibc (intentional abort) but I may be mistaken.

It should be easy to make a test case (on 64-bit systems at least) by
allocating 128k objects of size greater than the mmap threshold (128k
iirc) and then freeing all of the even-indexed ones.

Rich
Andreas Schwab Feb. 4, 2015, 8:39 a.m. UTC | #10
Rich Felker <dalias@libc.org> writes:

> Interesting. Unfortunately this makes it impossible for the
> application to observe the "valid memory was unable to be freed"
> condition that occurs when you can't split a vma.

Just like today (not reliably anyway).

Andreas.

Patch
diff mbox

diff --git a/NEWS b/NEWS
index c91b9fc..85b2948 100644
--- a/NEWS
+++ b/NEWS
@@ -10,15 +10,16 @@  Version 2.21
 * The following bugs are resolved with this release:
 
   6652, 10672, 12674, 12847, 12926, 13862, 14132, 14138, 14171, 14498,
-  15215, 15378, 15884, 16009, 16418, 16191, 16469, 16576, 16617, 16619,
-  16657, 16740, 16857, 17192, 17266, 17273, 17344, 17363, 17370, 17371,
-  17411, 17460, 17475, 17485, 17501, 17506, 17508, 17522, 17555, 17570,
-  17571, 17572, 17573, 17574, 17582, 17583, 17584, 17585, 17589, 17594,
-  17601, 17608, 17616, 17625, 17630, 17633, 17634, 17635, 17647, 17653,
-  17657, 17658, 17664, 17665, 17668, 17682, 17702, 17717, 17719, 17722,
-  17723, 17724, 17725, 17732, 17733, 17744, 17745, 17746, 17747, 17748,
-  17775, 17777, 17780, 17781, 17782, 17791, 17793, 17796, 17797, 17801,
-  17803, 17806, 17834, 17844, 17848, 17868, 17869, 17870, 17885, 17892.
+  15215, 15378, 15884, 16009, 16418, 16191, 16469, 16576, 16617, 16618,
+  16619, 16657, 16740, 16857, 17192, 17266, 17273, 17344, 17363, 17370,
+  17371, 17411, 17460, 17475, 17485, 17501, 17506, 17508, 17522, 17555,
+  17570, 17571, 17572, 17573, 17574, 17582, 17583, 17584, 17585, 17589,
+  17594, 17601, 17608, 17616, 17625, 17630, 17633, 17634, 17635, 17647,
+  17653, 17657, 17658, 17664, 17665, 17668, 17682, 17702, 17717, 17719,
+  17722, 17723, 17724, 17725, 17732, 17733, 17744, 17745, 17746, 17747,
+  17748, 17775, 17777, 17780, 17781, 17782, 17791, 17793, 17796, 17797,
+  17801, 17803, 17806, 17834, 17844, 17848, 17868, 17869, 17870, 17885,
+  17892.
 
 * A new semaphore algorithm has been implemented in generic C code for all
   machines. Previous custom assembly implementations of semaphore were
diff --git a/stdio-common/tst-sscanf.c b/stdio-common/tst-sscanf.c
index aece3f2..a12333c 100644
--- a/stdio-common/tst-sscanf.c
+++ b/stdio-common/tst-sscanf.c
@@ -233,5 +233,23 @@  main (void)
 	}
     }
 
+  /* BZ 16618 */
+  {
+#define SIZE 131072
+    CHAR *s = malloc ((SIZE + 1) * sizeof (*s));
+    if (s == NULL)
+      abort ();
+    for (size_t i = 0; i < SIZE; i++)
+      s[i] = L('0');
+    s[SIZE] = L('\0');
+    int i = 42;
+    if (SSCANF (s, L("%d"), &i) != 1)
+      result = 1;
+    if (i != 0)
+      result = 1;
+    free (s);
+#undef SIZE
+  }
+
   return result;
 }
diff --git a/stdio-common/vfscanf.c b/stdio-common/vfscanf.c
index cd129a8..c98a49e 100644
--- a/stdio-common/vfscanf.c
+++ b/stdio-common/vfscanf.c
@@ -274,9 +274,18 @@  _IO_vfscanf_internal (_IO_FILE *s, const char *format, _IO_va_list argptr,
 	  CHAR_T *old = wp;						    \
 	  size_t newsize = (UCHAR_MAX + 1 > 2 * wpmax			    \
 			    ? UCHAR_MAX + 1 : 2 * wpmax);		    \
-	  if (use_malloc || !__libc_use_alloca (newsize))		    \
+	  if (__glibc_unlikely (wpmax > SIZE_MAX / 2)			    \
+	      || __glibc_unlikely (newsize > SIZE_MAX / sizeof (CHAR_T)))   \
 	    {								    \
-	      wp = realloc (use_malloc ? wp : NULL, newsize);		    \
+	      if (use_malloc)						    \
+	    	free (old);						    \
+	      done = EOF;						    \
+	      __set_errno (ENOMEM);					    \
+	      goto errout;						    \
+	    }								    \
+	  if (use_malloc || !__libc_use_alloca (newsize * sizeof (CHAR_T))) \
+	    {								    \
+	      wp = realloc (use_malloc ? wp : NULL, newsize * sizeof (CHAR_T));	\
 	      if (wp == NULL)						    \
 		{							    \
 		  if (use_malloc)					    \