[PATCH FOR REVIEW] Potential Buffer Overflow in java_props_md.c

[Xueming Shen]

Andrew,

Since we are here:-) are we also supposed to "free" the old_temp at #250 
and old_temp
and old_ev at the end?

-Sherman

On 08/07/2012 04:49 AM, Andrew Hughes wrote:
> ----- Original Message -----
>> Thanks Andrew. 7189533 created for this.
>>
> Thanks.  Ok to push then? :-)
>
>> David
>> -----
>>
>> On 3/08/2012 8:03 AM, Andrew Hughes wrote:
>>>
>>> ----- Original Message -----
>>>> Hi Andrew,
>>>>
>>>> On 2/08/2012 7:35 PM, Andrew Hughes wrote:
>>>>> ----- Original Message -----
>>>>>> Andrew et al,
>>>>>>
>>>>>> AFAICS here:
>>>>>>
>>>>>>           220     encoding_variant = malloc(strlen(temp)+1);
>>>>>>           221     if (encoding_variant == NULL) {
>>>>>>           222         JNU_ThrowOutOfMemoryError(env, NULL);
>>>>>>           223         return 0;
>>>>>>           224     }
>>>>>>
>>>>>> we also need to do free(temp). Similarly later where we return
>>>>>> with
>>>>>> OOM
>>>>>> due to realloc failure, don't we also need to free what was
>>>>>> previously
>>>>>> malloc'd?
>>>>>>
>>>>> I was thinking the same just before committing, but didn't want
>>>>> to
>>>>> delay
>>>>> the main fix any further.
>>>>>
>>>>> While logically we do need one, I'm not sure it's worthwhile if
>>>>> we're going
>>>>> to throw the exception and exit anyway?  Will the return even be
>>>>> reached?
>>>>> If we're already near enough to OOM that we can't allocate more
>>>>> memory,
>>>>> it's unlikely freeing temp is going to get us much, and I presume
>>>>> it will
>>>>> be freed anyway when the VM process exists.
>>>> Are we definitely going to exit on this error? I agree if we're
>>>> out
>>>> of
>>>> memory we're likely to continue to run into problems if we try to
>>>> continue. But I'd prefer to see proper cleanup rather than making
>>>> assumptions about the context in which the code is used.
>>>>
>>>>> But I can add it if necessary.  It's trivial after all and does
>>>>> not
>>>>> real
>>>>> harm.
>>>> It will also prevent us getting flagged with memory-leak
>>>> warnings/errors
>>>> from static analysis tools.
>>>>
>>> This should cover it, I think:
>>>
>>> http://cr.openjdk.java.net/~andrew/buffer_overflow/webrev.02/
>>>
>>> but it'll need a bug ID.
>>>
>>>> Thanks,
>>>> David
>>>>
>>>>>> David
>>>>>>
>>>>>> On 2/08/2012 7:18 AM, Andrew Hughes wrote:
>>>>>>>
>>>>>>> ----- Original Message -----
>>>>>>>> On 01/08/2012 14:52, Andrew Hughes wrote:
>>>>>>>>> :
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> In any case, there is a Sun bug open for this:
>>>>>>>>>
>>>>>>>>> 6844255: Potential stack corruption in GetJavaProperties
>>>>>>>>>
>>>>>>>>> Can I take it that I can just get on and push Omair's
>>>>>>>>> extended
>>>>>>>>> version now then,
>>>>>>>>> with that bug ID?
>>>>>>>> Yes, go ahead, I should have said that in my mail.
>>>>>>>>
>>>>>>> Thanks.
>>>>>>>
>>>>>>> Done:
>>>>>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-August/010993.html
>>>>>>>
>>>>>>> with Omair as author and yourself and I as reviewers.
>>>>>>>
>>>>>>>>> Well, the locale can be set be an environment variable, so it
>>>>>>>>> could
>>>>>>>>> potentially
>>>>>>>>> be anything of any length...
>>>>>>>>>
>>>>>>>>> The Debian bug posted above has an example, though I couldn't
>>>>>>>>> replicate it.
>>>>>>>>>
>>>>>>>> I couldn't replicate it either and was just curious if anyone
>>>>>>>> managed
>>>>>>>> to
>>>>>>>> demonstrate it.
>>>>>>>>
>>>>>>> Yeah, I tend to think it's more potentially exploitable rather
>>>>>>> than
>>>>>>> something
>>>>>>> that's actually been hit.
>>>>>>>
>>>>>>>> -Alan.
>>>>>>>>
>>>>>>> Thanks,