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

[Xueming Shen]

On 8/7/12 5:33 PM, David Holmes wrote:
> On 8/08/2012 2:40 AM, Xueming Shen wrote:
>>
>> 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?
>
> No. They are aliases for temp and encoding_variant, which are freed at 
> the end. There are only ever at most two things to free.

oops, I did not see it's a realloc....


>
> Andrew: this is fine by me, but needs TL approval (Alan?)
>
> David
> -----
>
>>
>> -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,
>>