[8u] RFR for JDK-8157548: JVM crashes sometimes while starting

[Shafi Ahmad]

Hi Ioi,

Thanks for the review. Yes, you are right. 
I mentioned my earlier mail 'it may crash' I think  I should mentioned 'it may behave unexpectedly'. It totally dependent on the content of the buffer.     

s1 = ['j','\0','v','a','/','l','a',n',g',/ ',S',t',r',i',n',g']. //  Crash scenario , other possiblilties are ['j','a','\0','a','/','l','a',n',g',/ ',S',t',r',i',n',g'], ['j','a','v,'\0','/','l','a',n',g',/ ',S',t',r',i',n',g'] and ['j','a','v','a','\0','l','a',n',g',/ ',S',t',r',i',n',g'].  
s2 = "java/"
n = 5
As in the older code we are not checking length of buffer s1 so it returns true because s[1] = '\0' or s[2] = '\0'  or s[3] = '\0' or s[4] = '\0'  and it crashes later.

Assume below scenario:
s1 = ['j','a''v','a','/','l','a',n',g',/ ',S',t',r',i',n',g'].  // Assume s1 is pointing to same space as a freed copy of the Symbol "java/lang/String" but with size 4 and let 's1 + 4' is not accessible. Note there is no null character.   
s2 = "java/"
n = 5

In this scenario strncmp may crash when it tries to access memory location s1 + 4 as n is 5 and there is nothing to stop strncmp not to access 's1 + 4'

With the current code change we are avoiding accessing memory location which is not owned by s1.

Regards,
Shafi   

> -----Original Message-----
> From: Ioi Lam
> Sent: Tuesday, September 20, 2016 12:09 AM
> To: Shafi Ahmad; hotspot-dev at openjdk.java.net
> Subject: Re: [8u] RFR for JDK-8157548: JVM crashes sometimes while starting
> 
> Hi Shafi,
> 
> The fix is correct. However, the crash is not inside strncmp.
> 
> I reproduced this bug inside gdb. When the crash happened, the
> parsed_name Symbol is "j", which has one character. By luck, it occupies the
> same space as a freed copy of the Symbol "java/lang/String". For space
> saving, the string content in the Symbol is not nul terminated. Thus:
> 
>      strncmp((const char*)parsed_name->bytes(), pkg, strlen(pkg))
> 
> would return true. However, when we later call
> 
>      char* name = parsed_name->as_C_string();
>      char* index = strrchr(name, '/');
>     *index = '\0'; // <------- crash
> 
> name will be the 0-terminated string "j", and index would be NULL (because
> '/' is not contained inside name.) Storing into index would cause the SEGV.
> 
> Thanks
> - Ioi
> 
> 
> On 9/18/16 10:14 PM, Shafi Ahmad wrote:
> > Hi,
> >
> > Please review the small code change for bug: "JDK-8157548: JVM crashes
> > sometimes while starting" on jdk8u-dev
> >
> > Summary:
> > int strncmp(const char *s1, const char *s2, size_t n);
> >
> > s1 = "abcdefgh" // Assume this is not null terminated string.
> > s2 = "abcdefghijk"
> > n = 10
> >
> > In case if s1 is not null terminated then for above input strncmp may crash.
> >
> > In expression marked as (B) parsed_name->bytes() returns base address
> of non-null terminated string buffer.
> >
> > +  size_t pkglen = strlen(pkg);
> >     if (!HAS_PENDING_EXCEPTION &&
> >         !class_loader.is_null() &&
> >         parsed_name != NULL &&
> > -      !strncmp((const char*)parsed_name->bytes(), pkg, strlen(pkg))) {
> > +      parsed_name->utf8_length() >= (int)pkglen &&                             // -------
> ----------------------- (A)
> > +      !strncmp((const char*)parsed_name->bytes(), pkg, pkglen)) {
> > + //------------------------------ (B)
> >
> > Adding expression marked as (A) avoid the above similar input  scenario.
> >
> > Webrev: http://cr.openjdk.java.net/~shshahma/8157548/webrev.00/
> > Jdk8 bug: https://bugs.openjdk.java.net/browse/JDK-8157548
> >
> > Test:  Run jprt
> >
> > Note: Thanks to Ioi for providing the code change.
> >
> > Regards,
> > Shafi
>