| Message ID | CALoOobNFbi8csanuAGDwebQeojNWsSqj+6g6w-J94hZ8POOZiw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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.
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
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.
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
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
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.
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
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.
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) \
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.