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

[David Holmes]

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.

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