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

[Andrew Haley]

Alan Bateman wrote:
> Andrew Haley wrote:
>> https://bugs.openjdk.java.net/show_bug.cgi?id=100057
>>
>> GetJavaProperties has a stack-allocated fixed size buffer for holding
>> a copy of
>> a string returned by setlocale(3).  However, there is no guarantee
>> that the
>> string will fit into this buffer.
>>
>> This one is probably due to Solaris code being reused for Linux.  The
>> patch has been in IcedTea for a long while.
>>
>> OK to push, OpenJDK 7 and 6?

> I can't say I know this code very well but I see that the full-locale
> name can also be copied into temp when the locale is an alias. This
> makes me wonder if temp might need to be realloc'ed there? Also, I
> wonder if the return from malloc should be checked.
> 
> I've created a corresponding sunbug for this:
>  6844255: Potential stack corruption in GetJavaProperties

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.

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 17:37:20 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(p)+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);
         }
     }