Hi David, Alan,
So is the patch acceptable ?
It's webrev:
http://cr.openjdk.java.net/~zhouyx/7159982/webrev.02/
On Wed, Apr 18, 2012 at 12:15 PM, David Holmes <david.hol...@oracle.com>wrote:
> On 18/04/2012 1:15 PM, Sean Chou wrote:
>
>> 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<http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-April/009766.html>
>> .
>>
>
> Yes that is understood.
>
>
> 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.
>>
>
> I couldn't see why you changed ZIP_Get_From_Cache but now I see that it
> and ZIP_Put_In_Cache have to follow the same convention regarding the error
> string.
>
> Sorry for the confusion.
>
> David
> -----
>
> On Wed, Apr 18, 2012 at 10:43 AM, David Holmes <david.hol...@oracle.com
>> <mailto:david.holmes@oracle.**com <david.hol...@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.hol...@oracle.com
>> <mailto:david.holmes@oracle.**com<david.hol...@oracle.com>
>> >
>> <mailto:david.holmes@oracle.__**com
>>
>> <mailto:david.holmes@oracle.**com <david.hol...@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/%7E____zhouyx/7159982/webrev.02/>
>>
>> <http://cr.openjdk.java.net/~_**_zhouyx/7159982/webrev.02/<http://cr.openjdk.java.net/%7E__zhouyx/7159982/webrev.02/>
>> >
>>
>>
>>
>> <http://cr.openjdk.java.net/~_**_zhouyx/7159982/webrev.02/<http://cr.openjdk.java.net/%7E__zhouyx/7159982/webrev.02/>
>>
>> <http://cr.openjdk.java.net/~**zhouyx/7159982/webrev.02/<http://cr.openjdk.java.net/%7Ezhouyx/7159982/webrev.02/>
>> >>
>>
>> On Thu, Apr 12, 2012 at 4:24 PM, Alan
>> Bateman<Alan.Bateman@oracle.__**__com
>> <mailto:Alan.Bateman@oracle.__**com
>>
>> <mailto:Alan.Bateman@oracle.**com <alan.bate...@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/>
>> >>>**<
>> 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/
>>
>>
>> <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
>>
>>
--
Best Regards,
Sean Chou
