This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Fix _nl_find_msg malloc failure case, and callers.
- From: "Carlos O'Donell" <carlos at redhat dot com>
- To: Andreas Jaeger <aj at suse dot com>
- Cc: GNU C Library <libc-alpha at sourceware dot org>
- Date: Wed, 22 May 2013 14:56:00 -0400
- Subject: Re: [PATCH] Fix _nl_find_msg malloc failure case, and callers.
- References: <51892FCE dot 5030506 at redhat dot com> <518B4F31 dot 3050801 at suse dot com>
On 05/09/2013 03:24 AM, Andreas Jaeger wrote:
> On 05/07/2013 06:46 PM, Carlos O'Donell wrote:
>> Community,
>>
>> This patch fixes two issues, and perhaps should be two distinct patches,
>> but I present it here as one for the sake of completeness.
>>
>> This commit:
>> commit 006dd86111c44572dbd3b26e9c63dd0f834d7762
>> Author: Jeff Law <law at redhat.com>
>> Date: Thu Jun 21 17:15:38 2012 -0600
>>
>> [BZ #14277]
>> * intl/dcigettext.c (_nl_find_msg): Avoid use after potential
>> free. Simplify list management for _LIBC case.
>>
>> Fails to check malloc's return in intl/dcigettext.c (_nl_find_msg):
>> ~~~~
>> freemem_size = INITIAL_BLOCK_SIZE;
>> newmem = (transmem_block_t *) malloc (freemem_size);
>> # ifdef _LIBC
>> /* Add the block to the list of blocks we have to free
>> at some point. */
>> newmem->next = transmem_list;
>> transmem_list = newmem;
>> ~~~
>> If malloc fails then newmem is NULL then newmem->next results in a fault.
>> The fix is easy enough, check for newmem != NULL, and fall through to
>> the error condition below which returns (char *) -1 e.g. resource error.
>>
>> The problem is that returning (char *) -1 will break all sorts of other
>> code, so while what we did is correct, the real failure case fix is slightly
>> broader.
>>
>> There are 4 other places where _nl_find_msg is called:
>>
>> (a) Recursively in _nl_find_msg.
>>
>> Here we return -1 immediately if a recursive call to _nl_find_msg
>> returns -1. Resource errors are fatal at this point and if we
>> continue it just makes the matter worse by trying to allocate other
>> structure like conv_tab.
>>
>> (b) From _nl_load_domain.
>>
>> Here the error is also fatal, the domain is incomplete without
>> pluralization information and the failure in _nl_find_msg means
>> we won't be able to setup pluralization. We finalize the lock
>> we just initialized and exit via the normal error path.
>>
>> (c) Twice from DCIGETTEXT.
>>
>> In both cases the error is not fatal, it just means we have no
>> translation and return the original message. We patch the second
>> call to _nl_find_msg to ensure that future maintenance on the
>> function doesn't move the check for -1 further away.
>>
>> No regressions on x86-64 or x86.
>>
>> However, no regressions isn't really a useful metric for this code.
>>
>> The change was tested as documented here:
>> http://sourceware.org/glibc/wiki/Testing/WhiteBox
>>
>> OK to checkin?
>
> This looks fine, thanks,
After some more testing in Fedora rawhide...
commit 7a44c18fb4b1a65ebb1fece0b0d04f2570ed4d82
Author: Carlos O'Donell <carlos@redhat.com>
Date: Wed May 22 14:50:26 2013 -0400
Fix _nl_find_msg malloc failure case, and callers.
Checked in.
Cheers,
Carlos.