Code review request 6858865: Fix for 6728376 causes regression if the size of "data" is 0 and malloc returns Null for 0-length

[Xueming Shen]

On 11/19/2010 01:33 PM, Alan Bateman wrote:
> Xueming Shen wrote:
>> :
>>
>> I might not explain it clear enough. For Deflater.c we want to at 
>> least keep the "malloc"
>> path for
>>
>>     if ((*env)->GetBooleanField(env, this, setParamsID)) {
>>         ....
>>     }
>>
>> so it will continue go down to deflateParams() to reset the params, with
>> a valid non-NULL ptr on most platforms, even for 0-length buf, this is
>> what it was before #6728376 and what it is now, We don't what to have
>> another "regression" (change the params, invoke the deflate() with 
>> 0-length
>> buffer, but now it returns 0 with change the setting, I have to 
>> invoke it
>> again with...) here. And with the "len != 0" check, it will be no 
>> behavior
>> change as well for that weird OS.
>>
>> For other 2 paths (the Inflater.c and the
>>    (*env)->GetBooleanField(env, this, setParamsID) == false
>> path, I believe/think/guess it should be safe to bypath the zlib by
>> checking len/in_len and returning 0 if any of them is zero. But this is
>> again a "change", probably:-) harmless giving my understand/reading of
>> zlib code and the existing regression tests.
>>
>> So the "bit cleaner" webrev is
>> http://cr.openjdk.java.net/~sherman/6858865/webrev/
>>
>> But it deal with the 0-length issue "differently" at different places.
>> This is why I proposed last night the "safe" and "simple: change.
>>
>> Anyway, I'm fine with the "bit-cleaner" approach, so if you're OK with
>> it, throw it in:-)
>>
>> The "old" webrev is at
>> http://cr.openjdk.java.net/~sherman/6858865/webrev.00/
>>
>> -Sherman
>>
> The updated webrev looks better, but for the deflateParam path then 
> maybe you could do something like this:
>
> if (this_len == 0) {
>    in_buf = NULL;
> } else {
>    in_buf = (jbyte*) malloc(this_len);
>    if (in_buf == NULL) {
>        JNU_ThrowOutOfMemoryError(env, 0);
>        return 0;
>    }
> }
>
> and same thing for out_buf.
>
> Would that be a bit cleaner?

Why do you want to feed the zlib with a null buffer? :-)

The zlib#deflate() does some null checks at the beginning as showed below.
I probably can do something tricky to make it work, but for me I don't see
the benefit of taking the risk. If it works don't break it:-)

-Sherman



int ZEXPORT deflate (strm, flush)
     z_streamp strm;
     int flush;
{
     int old_flush; /* value of flush param for previous deflate call */
     deflate_state *s;

     if (strm == Z_NULL || strm->state == Z_NULL ||
         flush > Z_FINISH || flush < 0) {
         return Z_STREAM_ERROR;
     }
     s = strm->state;

     if (strm->next_out == Z_NULL ||
         (strm->next_in == Z_NULL && strm->avail_in != 0) ||
         (s->status == FINISH_STATE && flush != Z_FINISH)) {
         ERR_RETURN(strm, Z_STREAM_ERROR);
     }
     if (strm->avail_out == 0) ERR_RETURN(strm, Z_BUF_ERROR);
....

>
> -Alan
>
>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/core-libs-dev/attachments/20101119/9d1076c9/attachment.html>