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
.
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.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.hol...@oracle.com>
<mailto:david.holmes@oracle.__com
<mailto: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/~__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
<mailto: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/>><__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/>>>
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
