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

[David Holmes]

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.

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,
>>
>