Re: RFR 9: 8139390 : Very long classname in jimage causes SIGSEGV
> On Oct 29, 2015, at 4:22 PM, Roger Riggs wrote: > > Hi Mandy, > > Collapsing the emails. > > As for asserting that module name is never null; > it appears that the module name is not being supplied by the VM. (in hs-comp > or jdk9). > > On 10/29/15 12:59 PM, Mandy Chung wrote: >>> On Oct 27, 2015, at 11:40 PM, Roger Riggs wrote: >>> >>> Please review an update to the jimage reader implementation to correct the >>> case where a class name is very long causing a SEGV due to buffer overruns. >>> >>> The fix will be pushed to the hs-comp repo; the bug was first spotted there. >> I suggest to push it to jdk9/dev and that will be pulled into hs-comp when >> it’s sync’ed up. > ok, the issue was with HS tests failing and pushing to hs-comp more rapidly > addresses that. > But I'm fine with pushing to jdk9. > > (BTW, there is a different fix for this issue expected to be reimplemented > for Jake). >> >>> Webrev: >>> http://cr.openjdk.java.net/~rriggs//webrev-jimage-segv-8139390 >> Looks okay in general. >> >> ImageNativeSubstrate.cpp >> Is this native JIMAGE_FindResource method intended for tests to use? I >> don’t find any reference to it besides tests. The other option is to have a >> java method checking null parameters and call this native method (and make >> this native method private). > yes, it is only being used to specifically test the native JIMAGE_xxx native > functions. Since the fix will be reimplemented in jake, it would be better to hide this native method and do the null check in java. This would require existing tests to change. It’s okay with your version as an interim fix. >> >> test/jdk/internal/jimage/JImageReadTest.java >> 169 Assert.assertTrue(max > 16000, >> 170 "missing entries, should be more than 31000, reported: " >> + count); >> >> Is the change from 31000 to 16000 accidental? > When I added a test for a long classname, I discovered that the test was not > properly marked with @test > and was not being run. When I re-enabled it, I observed that the number of > entries > was quite a bit smaller than previously and so updated the test. I see. line 170 should be updated for the new number. >> >> This is unrelated to your change and just to mention it. The test hardcodes >> 9.0 as the version and the hardcoded value should be replaced for future >> release. Probably best to file a JBS issue for that. > I thought I had seen a change to allow a default version to be used. But at > the moment the > argument is ignored and until it is not ignored this value is immaterial. I filed https://bugs.openjdk.java.net/browse/JDK-8140803 to track this. Mandy
Re: RFR 9: 8139390 : Very long classname in jimage causes SIGSEGV
Hi Mandy, Collapsing the emails. As for asserting that module name is never null; it appears that the module name is not being supplied by the VM. (in hs-comp or jdk9). On 10/29/15 12:59 PM, Mandy Chung wrote: On Oct 27, 2015, at 11:40 PM, Roger Riggs wrote: Please review an update to the jimage reader implementation to correct the case where a class name is very long causing a SEGV due to buffer overruns. The fix will be pushed to the hs-comp repo; the bug was first spotted there. I suggest to push it to jdk9/dev and that will be pulled into hs-comp when it’s sync’ed up. ok, the issue was with HS tests failing and pushing to hs-comp more rapidly addresses that. But I'm fine with pushing to jdk9. (BTW, there is a different fix for this issue expected to be reimplemented for Jake). Webrev: http://cr.openjdk.java.net/~rriggs//webrev-jimage-segv-8139390 Looks okay in general. ImageNativeSubstrate.cpp Is this native JIMAGE_FindResource method intended for tests to use? I don’t find any reference to it besides tests. The other option is to have a java method checking null parameters and call this native method (and make this native method private). yes, it is only being used to specifically test the native JIMAGE_xxx native functions. test/jdk/internal/jimage/JImageReadTest.java 169 Assert.assertTrue(max > 16000, 170 "missing entries, should be more than 31000, reported: " + count); Is the change from 31000 to 16000 accidental? When I added a test for a long classname, I discovered that the test was not properly marked with @test and was not being run. When I re-enabled it, I observed that the number of entries was quite a bit smaller than previously and so updated the test. This is unrelated to your change and just to mention it. The test hardcodes 9.0 as the version and the hardcoded value should be replaced for future release. Probably best to file a JBS issue for that. I thought I had seen a change to allow a default version to be used. But at the moment the argument is ignored and until it is not ignored this value is immaterial. Thanks, Roger Mandy
Re: RFR 9: 8139390 : Very long classname in jimage causes SIGSEGV
> On Oct 27, 2015, at 11:40 PM, Roger Riggs wrote: > > Please review an update to the jimage reader implementation to correct the > case where a class name is very long causing a SEGV due to buffer overruns. > > The fix will be pushed to the hs-comp repo; the bug was first spotted there. I suggest to push it to jdk9/dev and that will be pulled into hs-comp when it’s sync’ed up. > > Webrev: > http://cr.openjdk.java.net/~rriggs//webrev-jimage-segv-8139390 Looks okay in general. ImageNativeSubstrate.cpp Is this native JIMAGE_FindResource method intended for tests to use? I don’t find any reference to it besides tests. The other option is to have a java method checking null parameters and call this native method (and make this native method private). test/jdk/internal/jimage/JImageReadTest.java 169 Assert.assertTrue(max > 16000, 170 "missing entries, should be more than 31000, reported: " + count); Is the change from 31000 to 16000 accidental? This is unrelated to your change and just to mention it. The test hardcodes 9.0 as the version and the hardcoded value should be replaced for future release. Probably best to file a JBS issue for that. Mandy
Re: RFR 9: 8139390 : Very long classname in jimage causes SIGSEGV
Resource file of zero-length module name and package name should not be allowed to be written to jimage. The moduleName > 0 and packageName > 0 should be an assertion; exception if fails. Mandy > On Oct 29, 2015, at 1:09 PM, Dmitry Samersoff > wrote: > > Roger, > > ImageNativeSubstrate.cpp:565 > >two extra bytes is accounted if moduleLen == 0 > > Please, add examples of valid resource name when moduleLen == 0 and/or > packageLen == 0 to comments. > > > -Dmitry > > On 2015-10-28 09:40, Roger Riggs wrote: >> Please review an update to the jimage reader implementation to correct the >> case where a class name is very long causing a SEGV due to buffer overruns. >> >> The fix will be pushed to the hs-comp repo; the bug was first spotted >> there. >> >> Webrev: >> http://cr.openjdk.java.net/~rriggs//webrev-jimage-segv-8139390 >> >> Issue: >> https://bugs.openjdk.java.net/browse/JDK-8139390 >> >> Thanks, Roger >> > > > -- > Dmitry Samersoff > Oracle Java development team, Saint Petersburg, Russia > * I would love to change the world, but they won't give me the sources.
Re: RFR 9: 8139390 : Very long classname in jimage causes SIGSEGV
Roger, ImageNativeSubstrate.cpp:565 two extra bytes is accounted if moduleLen == 0 Please, add examples of valid resource name when moduleLen == 0 and/or packageLen == 0 to comments. -Dmitry On 2015-10-28 09:40, Roger Riggs wrote: > Please review an update to the jimage reader implementation to correct the > case where a class name is very long causing a SEGV due to buffer overruns. > > The fix will be pushed to the hs-comp repo; the bug was first spotted > there. > > Webrev: >http://cr.openjdk.java.net/~rriggs//webrev-jimage-segv-8139390 > > Issue: >https://bugs.openjdk.java.net/browse/JDK-8139390 > > Thanks, Roger > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
Re: RFR 9: 8139390 : Very long classname in jimage causes SIGSEGV
Hi Alan, Yes, it uses the JNU_CHECK_EXCEPTION_RETURN macros. If it would be cleaner to remove the jni_util.h include, the macros can be replaced. They don't add much value in this case. Thanks, Roger On 10/28/15 8:15 AM, Alan Bateman wrote: On 28/10/2015 06:40, Roger Riggs wrote: Please review an update to the jimage reader implementation to correct the case where a class name is very long causing a SEGV due to buffer overruns. The fix will be pushed to the hs-comp repo; the bug was first spotted there. Webrev: http://cr.openjdk.java.net/~rriggs//webrev-jimage-segv-8139390 Issue: https://bugs.openjdk.java.net/browse/JDK-8139390 The new ThrowByName reminded me to ask if jni_util.h is needed. As libjimage doesn't link to libjava then there won't be any dependences but perhaps some of the macros are used? -Alan
Re: RFR 9: 8139390 : Very long classname in jimage causes SIGSEGV
On 28/10/2015 06:40, Roger Riggs wrote: Please review an update to the jimage reader implementation to correct the case where a class name is very long causing a SEGV due to buffer overruns. The fix will be pushed to the hs-comp repo; the bug was first spotted there. Webrev: http://cr.openjdk.java.net/~rriggs//webrev-jimage-segv-8139390 Issue: https://bugs.openjdk.java.net/browse/JDK-8139390 The new ThrowByName reminded me to ask if jni_util.h is needed. As libjimage doesn't link to libjava then there won't be any dependences but perhaps some of the macros are used? -Alan
