[PATCH] Fix addseverity (take 2)

[Jakub Jelinek <jakub at redhat dot com>]

On Tue, Jan 25, 2005 at 02:15:01PM -0800, Ulrich Drepper wrote:
> Jakub Jelinek wrote:
> >if the string passed to addseverity is freed by the caller,
> >we still reference freed up memory, or if the string passed to addseverity
> >is overwritten, we use whatever it was overwritten to.
> 
> I've never seen anything to the contrary that this is acceptable.  The 
> function is not part of any spec and when I wrote this code this seemed 
> to be what other implementations did.  What program has problems with 
> the current code?

Ok, it seems Solaris addseverity is grossly misdesigned - works ala putenv,
i.e. if you modify the string passed to it, it will change fmtmsg behaviour
and as glibc has this for compatibility only, let's be compatible.

I hope it is ok if we leak a few strings at exit time if SEV_LEVEL is set
in environment and the app ever calls fmtmsg - it wouldn't be hard to handle
that, but would mean using more memory and/or instructions for this obscure
interface just to shut up mtrace/valgrind.  (That leak is btw not introduced
by this patch, has been there like forever).

2005-01-25  Jakub Jelinek  <jakub@redhat.com>

	* stdlib/fmtmsg.c (addseverity): Remove new_string variable.
	(free_mem): Don't free string.
	* stdlib/tst-fmtmsg.c: Include string.h.
	(main): Add some more tests.

--- libc/stdlib/tst-fmtmsg.c.jj	2005-01-19 14:13:03.000000000 +0100
+++ libc/stdlib/tst-fmtmsg.c	2005-01-25 23:48:12.221066369 +0100
@@ -1,6 +1,7 @@
 #include <fmtmsg.h>
 #include <mcheck.h>
 #include <stdio.h>
+#include <string.h>
 
 
 #define MM_TEST 10
@@ -12,11 +13,13 @@ main (void)
 
   mtrace ();
 
-  if (addseverity (MM_TEST, "TEST") != MM_OK)
+  char TEST[] = "ABCD";
+  if (addseverity (MM_TEST, TEST) != MM_OK)
     {
       puts ("addseverity failed");
       result = 1;
     }
+  strcpy (TEST, "TEST");
 
   if (fmtmsg (MM_PRINT, "GLIBC:tst-fmtmsg", MM_HALT, "halt",
 	      "should print message for MM_HALT", "GLIBC:tst-fmtmsg:1")
@@ -54,5 +57,25 @@ main (void)
       result = 1;
     }
 
+  if (addseverity (MM_TEST, NULL) != MM_NOTOK)
+    {
+      puts ("third addseverity unexpectedly succeeded");
+      result = 1;
+    }
+
+  char *p = strdup ("TEST2");
+  if (addseverity (MM_TEST, p) != MM_OK)
+    {
+      puts ("fourth addseverity failed");
+      result = 1;
+    }
+  if (addseverity (MM_TEST, "TEST3") != MM_OK)
+    {
+      puts ("fifth addseverity failed");
+      result = 1;
+    }
+
+  free (p);
+
   return result;
 }
--- libc/stdlib/fmtmsg.c.jj	2005-01-25 22:30:23.000000000 +0100
+++ libc/stdlib/fmtmsg.c	2005-01-25 23:47:29.079775049 +0100
@@ -316,7 +316,7 @@ internal_addseverity (int severity, cons
   int result = MM_OK;
 
   /* First see if there is already a record for the severity level.  */
-  for (runp = severity_list, lastp = NULL; runp != NULL; runp = runp-> next)
+  for (runp = severity_list, lastp = NULL; runp != NULL; runp = runp->next)
     if (runp->severity == severity)
       break;
     else
@@ -364,34 +364,17 @@ int
 addseverity (int severity, const char *string)
 {
   int result;
-  const char *new_string;
 
   /* Prevent illegal SEVERITY values.  */
   if (severity <= MM_INFO)
     return MM_NOTOK;
 
-  if (string == NULL)
-    /* We want to remove the severity class.  */
-    new_string = NULL;
-  else
-    {
-      new_string = __strdup (string);
-
-      if (new_string == NULL)
-	/* Allocation failed or illegal value.  */
-	return MM_NOTOK;
-    }
-
   /* Protect the global data.  */
   __libc_lock_lock (lock);
 
   /* Do the real work.  */
   result = internal_addseverity (severity, string);
 
-  if (result != MM_OK)
-    /* Free the allocated string.  */
-    free ((char *) new_string);
-
   /* Release the lock.  */
   __libc_lock_unlock (lock);
 
@@ -408,7 +391,6 @@ libc_freeres_fn (free_mem)
       {
 	/* This is data we have to release.  */
 	struct severity_info *here = runp;
-	free ((char *) runp->string);
 	runp = runp->next;
 	free (here);
       }


	Jakub