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

["Carlos O'Donell" <carlos at redhat dot com>]

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?

2013-05-03  Carlos O'Donell  <carlos at redhat.com>

	* intl/dcigettext.c (DCIGETTEXT): Skip translating if _nl_find_msg returns -1.
	(_nl_find_msg): Return -1 if recursive call returned -1. If newmem is null
	return -1.
	* intl/loadmsgcat.c (_nl_load_domain): If _nl_find_msg returns -1 abort
	loading the domain.

diff --git a/intl/dcigettext.c b/intl/dcigettext.c
index 110307b..f4aa215 100644
--- a/intl/dcigettext.c
+++ b/intl/dcigettext.c
@@ -638,6 +638,11 @@ DCIGETTEXT (domainname, msgid1, msgid2, plural, n, category)
 		  retval = _nl_find_msg (domain->successor[cnt], binding,
 					 msgid1, 1, &retlen);
 
+		  /* Resource problems are not fatal, instead we return no
+		     translation.  */
+		  if (__builtin_expect (retval == (char *) -1, 0))
+		    goto no_translation;
+
 		  if (retval != NULL)
 		    {
 		      domain = domain->successor[cnt];
@@ -941,6 +946,11 @@ _nl_find_msg (domain_file, domainbinding, msgid, convert, lengthp)
 	    nullentry =
 	      _nl_find_msg (domain_file, domainbinding, "", 0, &nullentrylen);
 
+	    /* Resource problems are fatal.  If we continue onwards we will
+	       only attempt to calloc a new conv_tab and fail later.  */
+	    if (__builtin_expect (nullentry == (char *) -1, 0))
+	      return (char *) -1;
+
 	    if (nullentry != NULL)
 	      {
 		const char *charsetstr;
@@ -1170,10 +1180,14 @@ _nl_find_msg (domain_file, domainbinding, msgid, convert, lengthp)
 		      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 (newmem != NULL)
+			{
+			  /* Add the block to the list of blocks we have to free
+			     at some point.  */
+			  newmem->next = transmem_list;
+			  transmem_list = newmem;
+			}
+		      /* Fall through and return -1.  */
 # endif
 		    }
 		  if (__builtin_expect (newmem == NULL, 0))
diff --git a/intl/loadmsgcat.c b/intl/loadmsgcat.c
index e4b7b38..ac90ed1 100644
--- a/intl/loadmsgcat.c
+++ b/intl/loadmsgcat.c
@@ -1237,7 +1237,7 @@ _nl_load_domain (domain_file, domainbinding)
     default:
       /* This is an invalid revision.  */
     invalid:
-      /* This is an invalid .mo file.  */
+      /* This is an invalid .mo file or we ran out of resources.  */
       free (domain->malloced);
 #ifdef HAVE_MMAP
       if (use_mmap)
@@ -1257,6 +1257,11 @@ _nl_load_domain (domain_file, domainbinding)
 
   /* Get the header entry and look for a plural specification.  */
   nullentry = _nl_find_msg (domain_file, domainbinding, "", 0, &nullentrylen);
+  if (__builtin_expect (nullentry == (char *) -1, 0))
+    {
+      __libc_rwlock_fini (domain->conversions_lock);
+      goto invalid;
+    }
   EXTRACT_PLURAL_EXPRESSION (nullentry, &domain->plural, &domain->nplurals);
 
  out:
---

Cheers,
Carlos.