[PATCH] Fix addseverity

[PATCH] Fix addseverity

[Jakub Jelinek]

Hi!

As the testcase below shows, the right fix for addseverity is not
removing the free call in internal_addseverity, but to use new_string
that addseverity computes for exactly that purpose.  Otherwise, we leak
memory and 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.

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

	* stdlib/fmtmsg.c (internal_addseverity): Revert 2005-01-14 change.
	(addseverity): Pass new_string instead of string to internal_addseverity.
	* 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:03:49.864872564 +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[] = "TEST";
+  if (addseverity (MM_TEST, TEST) != MM_OK)
     {
       puts ("addseverity failed");
       result = 1;
     }
+  strcpy (TEST, "ABCD");
 
   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;
+    }
+  free (p);
+
+  if (addseverity (MM_TEST, "TEST3") != MM_OK)
+    {
+      puts ("fifth addseverity failed");
+      result = 1;
+    }
+
   return result;
 }
--- libc/stdlib/fmtmsg.c.jj	2005-01-25 22:30:23.000000000 +0100
+++ libc/stdlib/fmtmsg.c	2005-01-25 22:51:47.558972441 +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
@@ -324,6 +324,9 @@ internal_addseverity (int severity, cons
 
   if (runp != NULL)
     {
+      /* Release old string.  */
+      free ((char *) runp->string);
+
       if (string != NULL)
 	/* Change the string.  */
 	runp->string = string;
@@ -386,7 +389,7 @@ addseverity (int severity, const char *s
   __libc_lock_lock (lock);
 
   /* Do the real work.  */
-  result = internal_addseverity (severity, string);
+  result = internal_addseverity (severity, new_string);
 
   if (result != MM_OK)
     /* Free the allocated string.  */

	Jakub

Re: [PATCH] Fix addseverity

[Ulrich Drepper]

[-- Attachment #1: Type: text/plain, Size: 553 bytes --]

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?

-- 
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

[PATCH] Fix addseverity (take 2)

[Jakub Jelinek]

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

Re: [PATCH] Fix addseverity (take 2)

[Ulrich Drepper]

[-- Attachment #1: Type: text/plain, Size: 110 bytes --]

Applied.

-- 
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]