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

[Artem Ananiev]

On 1/25/2012 6:42 PM, Anthony Petrov wrote:
> Hi Kumar,
>
> You're right, this additional check and a comment make sense. I've
> updated the webrev at:
>
> http://cr.openjdk.java.net/~anthony/x-8-getSplashScreenNull.2/
>
> Please review.

Looks fine for me as well.

Thanks,

Artem

> --
> best regards,
> Anthony
>
> On 1/25/2012 5:51 PM, Kumar Srinivasan wrote:
>> 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
>>>>
>>