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 <[email protected]>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 <[email protected]
>> <mailto:david.holmes@oracle.**com <[email protected]>>> 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@oracle.__**com
>> <mailto:Alan.Bateman@oracle.**com <[email protected]>
>> >>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
