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

[David Holmes]

Thanks Andrew. 7189533 created for this.

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