Request for approval: Bug 100057 - Potential stack corruption in GetJavaProperties

[Andrew Haley]

Alan Bateman wrote:
> Andrew Haley wrote:
>> :
>> I've reworked the patch.
>>
>> I tried to refactor the code to make it cleaner and easier to follow,
>> but I ended up touching so many places that it bacame a major change,
>> and there's no way I could possibly test it all, especially the special
>> cases for Solaris.
>>
>> So, I backed off and produced this, the minimum patch.  encoding_variant
>> and temp are malloc()d to the size that they need, and temp is
>> realloc()d,
>> as you suggest.
>>   
> This looks much better except the size when allocating encoding_variant.
> If I read the patch correctly then encoding_variant is malloc'ed before
> 'p' is initialized (or 'p' might actually be null in the Solaris case
> when not a euro locale). Do I have this right?

Yes, you're right.  :-(

I reworked this silly patch so many times that I couldn't see it any
more.  As you say, encoding_variant needs to be at least the size of temp.

> strlen(temp)+1 would work  but would allocate a bit more than you need.
> A minor nit is that a malloc failure will return without freeing temp
> but I wouldn't worry about this because this is called once during>
> startup and we won't get too far if heap is completely exhausted.

Thanks for catching this.

Andrew.


diff -r 238591c80bc5 src/solaris/native/java/lang/java_props_md.c
--- a/src/solaris/native/java/lang/java_props_md.c	Thu May 21 18:41:50 2009 +0100
+++ b/src/solaris/native/java/lang/java_props_md.c	Fri May 22 18:38:21 2009 +0100
@@ -211,14 +211,19 @@
              * <language name>_<country name>.<encoding name>@<variant name>
              * <country name>, <encoding name>, and <variant name> are optional.
              */
-            char temp[64];
             char *language = NULL, *country = NULL, *variant = NULL,
                  *encoding = NULL;
             char *std_language = NULL, *std_country = NULL, *std_variant = NULL,
                  *std_encoding = NULL;
-            char *p, encoding_variant[64];
+            char *p, *encoding_variant;
             int i, found;

+            char *temp = malloc(strlen(lc)+1);
+            if (temp == NULL) {
+                JNU_ThrowOutOfMemoryError(env, NULL);
+                return NULL;
+            }
+
 #ifndef __linux__
             /*
              * Workaround for Solaris bug 4201684: Xlib doesn't like @euro
@@ -252,6 +257,13 @@
              * to a default country if that's possible.  It's also used to map
              * the Solaris locale aliases to their proper Java locale IDs.
              */
+
+            encoding_variant = malloc(strlen(temp)+1);
+            if (encoding_variant == NULL) {
+                JNU_ThrowOutOfMemoryError(env, NULL);
+                return NULL;
+            }
+
             if ((p = strchr(temp, '.')) != NULL) {
                 strcpy(encoding_variant, p); /* Copy the leading '.' */
                 *p = '\0';
@@ -263,7 +275,12 @@
             }

             if (mapLookup(locale_aliases, temp, &p)) {
-                strcpy(temp, p);
+                temp = realloc(temp, strlen(p)+1);
+                if (temp == NULL) {
+                    JNU_ThrowOutOfMemoryError(env, NULL);
+                    return NULL;
+                }
+		strcpy(temp, p);
             }

             language = temp;
@@ -357,6 +374,9 @@
 #endif
             sprops.encoding = std_encoding;
             sprops.sun_jnu_encoding = sprops.encoding;
+
+            free(temp);
+            free(encoding_variant);
         }
     }