Replace the static error string in ZIP_Put_In_Cache0 with on stack memory
David Holmes
david.holmes at oracle.com
Wed Apr 18 13:02:01 UTC 2012
On 18/04/2012 10:23 PM, Sean Chou wrote:
> Hi David, Alan,
>
> So is the patch acceptable ?
There is still the matter of the unexpected NULL if strdup fails. I'd
need to see the clients for this code to see how they handle failure. My
concern is the case where the caller sees a NULL return which indicates
an error, and so accesses the msg and now potentially hits another NULL.
It's unlikely but ...
David
> It's webrev:
> http://cr.openjdk.java.net/~zhouyx/7159982/webrev.02/
> <http://cr.openjdk.java.net/%7Ezhouyx/7159982/webrev.02/>
>
> On Wed, Apr 18, 2012 at 12:15 PM, David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at 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.holmes at oracle.com <mailto:david.holmes at oracle.com>
> <mailto: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>>
> <mailto:david.holmes at oracle. <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/%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 at oracle.______com
> <mailto:Alan.Bateman at oracle. <mailto: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/~** <http://cr.openjdk.java.net/%7E**>
>
>
> 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/ <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/
> <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
>
More information about the core-libs-dev
mailing list