Replace the static error string in ZIP_Put_In_Cache0 with on stack memory
David Holmes
david.holmes at oracle.com
Wed Apr 18 04:15:26 UTC 2012
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.holmes at oracle.com
> <mailto: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>
> <mailto:david.holmes at oracle.__com
> <mailto: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
> <mailto: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/
> <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
>
More information about the core-libs-dev
mailing list