Replace the static error string in ZIP_Put_In_Cache0 with on stack memory

[David Holmes]

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>> 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/>
>
>         On Thu, Apr 12, 2012 at 4:24 PM, Alan
>         Bateman<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/><ht__tp://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
>