> On Oct 29, 2015, at 4:22 PM, Roger Riggs <roger.ri...@oracle.com> 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 <roger.ri...@oracle.com> 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
