Replace the static error string in ZIP_Put_In_Cache0 with on stack memory

[Sean Chou]

Hi David,

    The current implementation uses static char array to keep the error
message, so it is possible when two errors happen at the same time, the
error message will be modified. I have a testcase attached in
http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-April/009766.html .

    So in the patch, the static char array is modified to on stack char
array to avoid the race in error case; and strdup is called because the
error message is currently kept on stack. But I didn't notice the case that
strdup might fail.

On Wed, Apr 18, 2012 at 10:43 AM, David Holmes <david.holmes at oracle.com>wrote:

> Hi Sean,
>
>
> On 18/04/2012 12:37 PM, Sean Chou wrote:
>
>>     To free the error string in ZIP_Open is a result of discussion with
>> hotspot. They said the error string is never used and they do not want
>> to do the free work in hotspot for ZIP_Open...
>>
>
> Ok. I assume there are no other callers of this method.
>
>
>      strdup would cause a NULL error string if memory allocation is
>> failed. If strdup is not used, another choice may be asking the caller
>> to reserve the space for error string. Caller can reserve the space on
>> stack, so *pmsg can still be set to NULL in ZIP_Put_In_Cache0 and caller
>> can keep the code for error handling. But this is also strange. Do you
>> have any better solutions?
>>
>
> I'm still unclear why the strdup is being used on string literals. Are we
> concerned with someone modifying the contents of the string literals?
>
>
>      It will not cause SEGV, there are NULL checks before free.
>>
>
> It is not the free that I'm worried about. If an error occurs but the
> strdup fails due to a malloc failure then the caller may reference the msg.
> Previously this msg was never NULL but now it may be.
>
> David
> -----
>
>  On Tue, Apr 17, 2012 at 10:48 AM, David Holmes <david.holmes at oracle.com
>> <mailto:david.holmes at oracle.**com <david.holmes at oracle.com>>> wrote:
>>
>>    Certainly the string management in this code is a bit of a mess, but
>>    I don't understand why the strdup's of string literals have been
>>    introduced. Even if for a good reason this seems to imply that an
>>    allocation failure will result in a NULL where before the caller was
>>    guaranteed never to get NULL in the error case, and that could lead
>>    to SEGV.
>>
>>    Also with the change to avoid changes on the hotspot side, the
>>    actual cause of the open failure has been lost in ZIP_Open
>>
>>    David
>>
>>
>>    On 13/04/2012 1:14 AM, Sean Chou wrote:
>>
>>        Hi Alan,
>>
>>             I made a new webrev, added the comments and the 2 other
>>        modification.
>>        It's now :
>>        http://cr.openjdk.java.net/~__**zhouyx/7159982/webrev.02/<http://cr.openjdk.java.net/~__zhouyx/7159982/webrev.02/>
>>
>>        <http://cr.openjdk.java.net/~**zhouyx/7159982/webrev.02/<http://cr.openjdk.java.net/~zhouyx/7159982/webrev.02/>
>> >
>>
>>        On Thu, Apr 12, 2012 at 4:24 PM, Alan
>>        Bateman<Alan.Bateman at oracle.__**com
>>        <mailto:Alan.Bateman at oracle.**com <Alan.Bateman at oracle.com>
>> >>wrote:
>>
>>
>>            On 12/04/2012 06:40, Sean Chou wrote:
>>
>>                Hi Alan,
>>
>>                    Many thanks.
>>
>>                    I updated the patch, ZIP_Open frees the error
>>                message and set "Zip
>>                file open error".
>>
>>                The new webrev is : http://cr.openjdk.java.net/~**
>>                zhouyx/7159982/webrev.01/<**http__://cr.openjdk.java.net/%
>> **__7Ezhouyx/7159982/webrev.01/
>>                <http://cr.openjdk.java.net/%**7Ezhouyx/7159982/webrev.01/<http://cr.openjdk.java.net/%7Ezhouyx/7159982/webrev.01/>
>> >><
>>                http://cr.openjdk.java.net/%****
>> __7Ezhouyx/7159982/webrev.01/
>>                <http://cr.openjdk.java.net/%***
>> *7Ezhouyx/7159982/webrev.01/><**ht__tp://cr.openjdk.java.net/%**
>> __7Ezhouyx/7159982/webrev.01/
>>
>>                <http://cr.openjdk.java.net/%**7Ezhouyx/7159982/webrev.01/<http://cr.openjdk.java.net/%7Ezhouyx/7159982/webrev.01/>
>> >>
>>
>>
>>
>>
>>                    Please take a look once more.
>>
>>            This looks much better. I think we'll need to add comments
>>            to the ZIP_*
>>            functions so that it's clear to anyone using them when they
>>            need to free
>>            the error message and then they don't.
>>
>>            One implementation nit at zip_util.c L876 where it should
>>            check if pmsg is
>>            NULL and I think the tests should be reversed so that its:
>>
>>            if (file != NULL&&  pmsg != NULL&&  *pmsg != NULL)  { ... }
>>
>>
>>            One other minor nit is L875 where there is a space on either
>>            side of the
>>            "*", best to keep the style consistent.
>>
>>            -Alan.
>>
>>
>>
>>
>>
>>
>>
>> --
>> Best Regards,
>> Sean Chou
>>
>>


-- 
Best Regards,
Sean Chou