Replace the static error string in ZIP_Put_In_Cache0 with on stack memory
Hi all, I would like to suggest to replace the static error string in ZIP_Put_In_Cache0 with on stack memory. Because it takes more time to modify a string than an int typed error number, it opens a window larger than int typed error number and might bring in race in multi-thread user cases. I wrote a simple c testcase for JNI call "ZIP_Open", which shows that it is easy to cause a race. In my machine, the testcase fails several time with only 2 threads. The testcase is attached, it needs jdk shared libs to compile and run. compile: gcc test_zip_open.c -I j2sdk-image/include/ -I j2sdk-image/include/linux/ j2sdk-image/jre/lib/i386/lib*.so j2sdk-image/jre/lib/i386/client/libjvm.so -pthread run: please include the libs in LD_LIBRARY_PATH , it is easy to catch the error case with grep //// Following is a simple output from my tests: zhouyx@zhouyx-workstation:/media/data/GiveBack/OJDK-482/i386$ ./a.out 1 | grep //// ////////////////////////// method1 pmsg :Permission denied zhouyx@zhouyx-workstation:/media/data/GiveBack/OJDK-482/i386$ ./a.out 10 | grep //// //////////////////////// method2 pmsg :No such file or directory //////////////////////// method2 pmsg :No such file or directory //////////////////////// method2 pmsg :No such file or directory zhouyx@zhouyx-workstation:/media/data/GiveBack/OJDK-482/i386$ ./a.out 100 | grep //// ////////////////////////// method1 pmsg :Permission denied ////////////////////////// method1 pmsg :Permission denied //////////////////////// method2 pmsg :No such file or directory //////////////////////// method2 pmsg :No such file or directory ////////////////////////// method1 pmsg :Permission denied The modification is here: http://cr.openjdk.java.net/~zhouyx/OJDK-482/webrev.00/ . It puts the error string on stack and return a dup one. Please take a look and give some comments, thanks. -- Best Regards, Sean Chou
On 09/04/2012 08:53, Sean Chou wrote:
Hi all,
I would like to suggest to replace the static error string in ZIP_Put_In_Cache0 with on stack memory.
:
The modification is here: http://cr.openjdk.java.net/~zhouyx/OJDK-482/webrev.00/ .
Good catch, this one had probably been there for a long time but doesn't seem to have been noticed (perhaps because it should be rare for 2+ threads to attempt to open malformed zip files at around the same time). The changes in the webrev look okay to me. Minor nit in zip_util.c at L847 where there should be a space in "if(". In ZipFile.c then another choice would be to add the free after ThrowZipException(env,msg). Where you have it is okay too but probably should be split over two lines to be consistent. I assume that Neil or Charles will push this for you. I've created a bug for it: 7159982: ZipFile uses static for error message when malformed zip file encountered Regards, Alan.
Hi Alan, I modified the patch according to your comments. The new webrev for cl is: http://cr.openjdk.java.net/~zhouyx/7159982/webrev.00/ And it is found that hotspot calls ZIP_Open through (*ZipOpen) in file classLoader.cpp .So I also made a patch for it and add hotspot-dev to cc list. File classLoader.cpp is the only one I have found calling ZIP_Open. The webrev for hotspot is: http://cr.openjdk.java.net/~zhouyx/7159982/webrev-hotspot.00/ To hotspot guys, We are trying to make the error path in ZIP_Open in src/share/native/java/util/zip/zip_util.c thread safe by changing the errbuf[] from static array to on stack array. This will cause the returned error string allocated from heap, which need to be freed. I checked the code and found only classLoader.cpp calls this function, so I made the above webrev. Please take a look. Link to the start of discussion: http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-April/009766.html Link to the bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7159982 On Mon, Apr 9, 2012 at 6:01 PM, Alan Bateman <Alan.Bateman@oracle.com>wrote:
On 09/04/2012 08:53, Sean Chou wrote:
Hi all,
I would like to suggest to replace the static error string in ZIP_Put_In_Cache0 with on stack memory.
:
The modification is here: http://cr.openjdk.java.net/~**zhouyx/OJDK-482/webrev.00/<http://cr.openjdk.java.net/~zhouyx/OJDK-482/webrev.00/> .
Good catch, this one had probably been there for a long time but doesn't seem to have been noticed (perhaps because it should be rare for 2+ threads to attempt to open malformed zip files at around the same time).
The changes in the webrev look okay to me. Minor nit in zip_util.c at L847 where there should be a space in "if(". In ZipFile.c then another choice would be to add the free after ThrowZipException(env,msg). Where you have it is okay too but probably should be split over two lines to be consistent.
I assume that Neil or Charles will push this for you. I've created a bug for it: 7159982: ZipFile uses static for error message when malformed zip file encountered
Regards, Alan.
-- Best Regards, Sean Chou
On 10/04/2012 08:10, Sean Chou wrote:
And it is found that hotspot calls ZIP_Open through (*ZipOpen) in file classLoader.cpp .So I also made a patch for it and add hotspot-dev to cc list. File classLoader.cpp is the only one I have found calling ZIP_Open.
The webrev for hotspot is: http://cr.openjdk.java.net/~zhouyx/7159982/webrev-hotspot.00/ <http://cr.openjdk.java.net/%7Ezhouyx/7159982/webrev-hotspot.00/>
To hotspot guys,
We are trying to make the error path in ZIP_Open in src/share/native/java/util/zip/zip_util.c thread safe by changing the errbuf[] from static array to on stack array. This will cause the returned error string allocated from heap, which need to be freed. I checked the code and found only classLoader.cpp calls this function, so I made the above webrev. Please take a look.
It's good that you spotted that this is called from the VM (I'd forgotten that). In that case things get a bit more complicated because the hotspot repository takes a different route into master and also goes into 7u releases too. I'll leave the HotSpot group to comment on your proposed change but I assume it will require adding JDK_Version::is_gte_jdk18x_version so that the JDK version can be tested before decided whether to free the error message. It will also require a sponsor as you cannot push changes to jdk8/tl/hotspot, all HotSpot changes require going through a build+test system (no direct pushes). -Alan
Hi hotspot guys, Would any one like to take a look at this? I'm trying to fix a potential race in ZIP_Open, it is found classLoader.cpp uses this function. So a webrev for hotspot is made as well, but I need a sponsor from hotspot as suggested by Alan Bateman. The start of this thread is http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-April/009766.html. The webrevs: http://cr.openjdk.java.net/~zhouyx/7159982/webrev-hotspot.00/ and http://cr.openjdk.java.net/~zhouyx/7159982/webrev.00/ . And, I found JDK_Version::is_gte_jdk18x_version suggested by Alan does not exist in hotspot yet, shall I add it to the patch too ? On Tue, Apr 10, 2012 at 4:10 PM, Alan Bateman <Alan.Bateman@oracle.com>wrote:
On 10/04/2012 08:10, Sean Chou wrote:
And it is found that hotspot calls ZIP_Open through (*ZipOpen) in file classLoader.cpp .So I also made a patch for it and add hotspot-dev to cc list. File classLoader.cpp is the only one I have found calling ZIP_Open.
The webrev for hotspot is: http://cr.openjdk.java.net/~**zhouyx/7159982/webrev-hotspot.**00/<http://cr.openjdk.java.net/%7Ezhouyx/7159982/webrev-hotspot.00/><; http://cr.openjdk.java.net/%**7Ezhouyx/7159982/webrev-**hotspot.00/<http://cr.openjdk.java.net/%7Ezhouyx/7159982/webrev-hotspot.00/>
To hotspot guys,
We are trying to make the error path in ZIP_Open in src/share/native/java/util/**zip/zip_util.c thread safe by changing the errbuf[] from static array to on stack array. This will cause the returned error string allocated from heap, which need to be freed. I checked the code and found only classLoader.cpp calls this function, so I made the above webrev. Please take a look.
It's good that you spotted that this is called from the VM (I'd
forgotten that). In that case things get a bit more complicated because the hotspot repository takes a different route into master and also goes into 7u releases too. I'll leave the HotSpot group to comment on your proposed change but I assume it will require adding JDK_Version::is_gte_jdk18x_**version so that the JDK version can be tested before decided whether to free the error message. It will also require a sponsor as you cannot push changes to jdk8/tl/hotspot, all HotSpot changes require going through a build+test system (no direct pushes).
-Alan
-- Best Regards, Sean Chou
On 11/04/2012 15:58, Sean Chou wrote:
Hi hotspot guys,
Would any one like to take a look at this? I'm trying to fix a potential race in ZIP_Open, it is found classLoader.cpp uses this function. So a webrev for hotspot is made as well, but I need a sponsor from hotspot as suggested by Alan Bateman.
The start of this thread is http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-April/009766.html . The webrevs: http://cr.openjdk.java.net/~zhouyx/7159982/webrev-hotspot.00/ <http://cr.openjdk.java.net/%7Ezhouyx/7159982/webrev-hotspot.00/> and http://cr.openjdk.java.net/~zhouyx/7159982/webrev.00/ <http://cr.openjdk.java.net/%7Ezhouyx/7159982/webrev.00/> .
And, I found JDK_Version::is_gte_jdk18x_version suggested by Alan does not exist in hotspot yet, shall I add it to the patch too ? I had a brief chat with Coleen Phillimore about this. One suggestion that would avoid HotSpot changes is to change ZIP_Open so that it doesn't return an error message or returns something fixed. It's not used in the VM anyway (as per Coleen's reply). We need the error message for the java.util.zip APIs but that doesn't go through ZIP_Open so it means it can be freed and *pmsg = NULL returned (or a pointer to one fixed generic message).
-Alan
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/ Please take a look once more. On Thu, Apr 12, 2012 at 4:09 AM, Alan Bateman <Alan.Bateman@oracle.com>wrote:
On 11/04/2012 15:58, Sean Chou wrote:
Hi hotspot guys,
Would any one like to take a look at this? I'm trying to fix a potential race in ZIP_Open, it is found classLoader.cpp uses this function. So a webrev for hotspot is made as well, but I need a sponsor from hotspot as suggested by Alan Bateman.
The start of this thread is 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>. The webrevs: http://cr.openjdk.java.net/~** zhouyx/7159982/webrev-hotspot.**00/<http://cr.openjdk.java.net/~zhouyx/7159982/webrev-hotspot.00/><; http://cr.openjdk.java.net/%**7Ezhouyx/7159982/webrev-**hotspot.00/<http://cr.openjdk.java.net/%7Ezhouyx/7159982/webrev-hotspot.00/>
and http://cr.openjdk.java.net/~**zhouyx/7159982/webrev.00/<http://cr.openjdk.java.net/~zhouyx/7159982/webrev.00/><; http://cr.openjdk.java.net/%**7Ezhouyx/7159982/webrev.00/<http://cr.openjdk.java.net/%7Ezhouyx/7159982/webrev.00/>> .
And, I found JDK_Version::is_gte_jdk18x_**version suggested by Alan does not exist in hotspot yet, shall I add it to the patch too ?
I had a brief chat with Coleen Phillimore about this. One suggestion that would avoid HotSpot changes is to change ZIP_Open so that it doesn't return an error message or returns something fixed. It's not used in the VM anyway (as per Coleen's reply). We need the error message for the java.util.zip APIs but that doesn't go through ZIP_Open so it means it can be freed and *pmsg = NULL returned (or a pointer to one fixed generic message).
-Alan
-- Best Regards, Sean Chou
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/>
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.
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/ On Thu, Apr 12, 2012 at 4:24 PM, Alan Bateman <Alan.Bateman@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/>
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
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/
On Thu, Apr 12, 2012 at 4:24 PM, Alan Bateman<Alan.Bateman@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/>
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.
Hi David, 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... 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? It will not cause SEGV, there are NULL checks before free. On Tue, Apr 17, 2012 at 10:48 AM, David Holmes <david.holmes@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@oracle.**com<Alan.Bateman@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/<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
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@oracle.com <mailto:david.holmes@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@oracle.__com <mailto:Alan.Bateman@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
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 . 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. On Wed, Apr 18, 2012 at 10:43 AM, David Holmes <david.holmes@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@oracle.com
<mailto:david.holmes@oracle.**com <david.holmes@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@oracle.__**com <mailto:Alan.Bateman@oracle.**com <Alan.Bateman@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/><**ht__tp://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
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@oracle.com <mailto:david.holmes@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@oracle.com <mailto:david.holmes@oracle.com> <mailto:david.holmes@oracle.__com <mailto:david.holmes@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.Bateman@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
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.holmes@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@oracle.com
<mailto:david.holmes@oracle.**com <david.holmes@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@oracle.com <mailto:david.holmes@oracle.**com<david.holmes@oracle.com>
<mailto:david.holmes@oracle.__**com
<mailto:david.holmes@oracle.**com <david.holmes@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.Bateman@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/
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
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@oracle.com <mailto:david.holmes@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.h... <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@oracle.com <mailto:david.holmes@oracle.com> <mailto:david.holmes@oracle.__com <mailto:david.holmes@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@oracle.com <mailto:david.holmes@oracle.com> <mailto:david.holmes@oracle.__com <mailto:david.holmes@oracle.com>> <mailto:david.holmes@oracle. <mailto:david.holmes@oracle.>____com
<mailto:david.holmes@oracle.__com <mailto:david.holmes@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. <mailto:Alan.Bateman@oracle.>____com
<mailto:Alan.Bateman@oracle.__com <mailto:Alan.Bateman@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
On 18/04/2012 14:02, David Holmes wrote:
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 ... I looked through the usages and nothing obvious jumps out. HotSpot will invoke ZIP_Open and that now returns a fixed string in the event of a failure. The library code opens the zip file directly and then uses ZIP_Put_In_Cache0 which seems to be handling this case.
-Alan.
On 19/04/2012 4:05 AM, Alan Bateman wrote:
On 18/04/2012 14:02, David Holmes wrote:
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 ... I looked through the usages and nothing obvious jumps out. HotSpot will invoke ZIP_Open and that now returns a fixed string in the event of a failure. The library code opens the zip file directly and then uses ZIP_Put_In_Cache0 which seems to be handling this case.
Yes looks like we only have two bits of client code here and ZipFile.c checks for NULL. Ok. David -----
-Alan.
Thanks David and Alan, shall I find some one to commit it ? On Thu, Apr 19, 2012 at 8:53 AM, David Holmes <david.holmes@oracle.com>wrote:
On 19/04/2012 4:05 AM, Alan Bateman wrote:
On 18/04/2012 14:02, David Holmes wrote:
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 ...
I looked through the usages and nothing obvious jumps out. HotSpot will invoke ZIP_Open and that now returns a fixed string in the event of a failure. The library code opens the zip file directly and then uses ZIP_Put_In_Cache0 which seems to be handling this case.
Yes looks like we only have two bits of client code here and ZipFile.c checks for NULL.
Ok.
David -----
-Alan.
-- Best Regards, Sean Chou
On 19/04/2012 09:05, Sean Chou wrote:
Thanks David and Alan, shall I find some one to commit it ?
I assume Charles or Neil will push it for you. If they aren't available then I should be able to find someone to sponsor this. -Alan.
On 04/19/2012 04:05 PM, Sean Chou wrote:
Thanks David and Alan, shall I find some one to commit it ?
On Thu, Apr 19, 2012 at 8:53 AM, David Holmes<david.holmes@oracle.com>wrote:
On 19/04/2012 4:05 AM, Alan Bateman wrote:
On 18/04/2012 14:02, David Holmes wrote:
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 ...
I looked through the usages and nothing obvious jumps out. HotSpot will invoke ZIP_Open and that now returns a fixed string in the event of a failure. The library code opens the zip file directly and then uses ZIP_Put_In_Cache0 which seems to be handling this case.
Yes looks like we only have two bits of client code here and ZipFile.c checks for NULL.
Ok.
David -----
-Alan.
Hi Sean, The patch is committed @ Changeset: c3905c1f5da7 Author: zhouyx Date: 2012-04-20 16:11 +0800 URL:http://hg.openjdk.java.net/jdk8/tl/jdk/rev/c3905c1f5da7 7159982: ZipFile uses static for error message when malformed zip file encountered Reviewed-by: alanb, dholmes Thank you all for reviewing this. -- Yours Charles
participants (4)
-
Alan Bateman
-
Charles Lee
-
David Holmes
-
Sean Chou