Re: [PATCH] Fix _nl_find_msg malloc failure case, and callers.

[Andreas Jaeger <aj at suse dot com>]

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,

Andreas
--
 Andreas Jaeger aj@{suse.com,opensuse.org} Twitter/Identica: jaegerandi
  SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
   GF: Jeff Hawn,Jennifer Guild,Felix Imendörffer,HRB16746 (AG Nürnberg)
    GPG fingerprint = 93A3 365E CE47 B889 DF7F  FED1 389A 563C C272 A126