Review request for 7129420: [macosx] SplashScreen.getSplashScreen() returns null

[Kumar Srinivasan]

Hi Anthony,

Generally looks good, however few more comments.

Shouldn't we check for negative returns from snprintf ?

The snprintf(3) contract indicates on openbsd that it will return <0 values
for errors, in which case,  we could be feeding an unitialized
splashPath to dlopen, perhaps we should initialize it as well ?

Also appreciate including your comments  below in the platform
source(s) if applicable, explaining why  we don't check the return
value for dlopen, will be good.  This is  in case someone decides
to to be meticulous and adds a check.

Thanks

Kumar
[PS: cc'ing Joe and David Holmes on launcher fixes, they are
usually interested in launcher changes]

> Hi Kumar,
>
> Thanks for refactoring the Java launcher code! I've just re-based my 
> fix to the new structure of the Mac OS X launcher sources, here it is:
>
> http://cr.openjdk.java.net/~anthony/x-8-getSplashScreenNull.1/
>
> I've applied all the suggestions listed below. Thanks for the review!
>
> Regarding your question about checking for dlopen() status - yes, 
> we're OK if the splash screen library is unable to load. E.g. Embedded 
> folks often remove some unnecessary parts from a JRE image to reduce 
> its size. If they remove the splash screen dynamic library we'll just 
> silently ignore the error and continue w/o showing the splash screen - 
> not that it's much needed for embedded applications anyway.
>
> The path lengths, however, are true error conditions, that's why I 
> JLI_ReportErrorMessage() them with my fix.
>
> -- 
> best regards,
> Anthony
>
> On 1/20/2012 9:48 PM, Kumar Srinivasan wrote:
>> Hi Anthony,
>>
>> Do you want to wait until I push the launcher changes which
>> are under review ?
>>
>> In the launcher code we tend to use the JLI prefixed posix
>> calls, ex: JLI_Snprintf. vs snprintf
>>
>> The error messages are defined in emessages.h, this is so
>> that we can L10N these messages in the future, if such a
>> requirement arises. I think you can use JRE_ERROR11
>>
>> Personally I would create two more buffers to avoid errors
>> and make it safer, these are a cause of some grief when
>> security audits are performed for buffer overruns.
>>
>> char tail[PATH_MAX]
>> char tmp[PATH_MAX]
>>
>> JLI_Snprintf(tail, PATH_MAX, "/lib/%s", SPLASHSCREEN_SO);
>>
>> if (JLI_Snprintf(tmp, PATH_MAX, "%s%s", path, tail)) {
>>  JLI_ReportErrorMessage.......
>> }
>>
>> also I noticed that there is no check for dlopen, so is it ok for the
>> splashscreen to fail silently if dlopen fails ?
>>
>> Thanks
>> Kumar
>>
>>
>>> Hello,
>>>
>>> Please review a fix for 
>>> http://bugs.sun.com/view_bug.do?bug_id=7129420 at:
>>>
>>> http://cr.openjdk.java.net/~anthony/x-8-getSplashScreenNull.0/
>>>
>>> We remove our custom mechanism to determine the splash screen 
>>> dynamic library path and instead start using the GetJREPath() for 
>>> this purpose. I've verified this fix with an SDK (not JRE!) image, 
>>> and it works just fine.
>>>
>>> -- 
>>> best regards,
>>> Anthony
>>