Approved.

-phil.

On 5/15/2014 9:31 AM, David DeHaven wrote:
Ping!

Does this look OK?

I've also filed an issue against JavaFX:
https://javafx-jira.kenai.com/browse/RT-37125

-DrD-

I tried not modifying libpng but still ended up with lingering references to 
longjmp in pngread.o, despite libpng having png_ptr->longjmp_fn (bug in 
libpng?). pngread.c calls setjmp to set a default location to jump to in case the 
caller doesn't call setjmp, so if we continue down this path something in libpng 
must be modified. The only other option is to create our own setjmp.h and order it 
before /usr/include/setjmp.h, which seems dubious at best.

I'm curious if the libpng changes are even needed since it's only used for 
splashscreen, which happens very early in the launch process. Also note that we 
didn't originally even call png_set_longjmp_fn, so any error should have 
resulted in an abort() instead of a call to longjmp... it appears we could 
retain the functionality we have today and #undef PNG_SETJMP_SUPPORTED 
(pngconf.h?). That would put the onus on developers to make sure their pngs 
don't have errors in them, or libsplashscreen will abort()...


That's an interesting question and the answer might extend to the splashscreen 
changes too.
Its platform specific code and on MAC, the thread is created using pthreads 
directly and that
thread goes away once splashscreen is done. But its running at the same time as 
the VM
is booting up and creating threads and setting their signal masks. So I don't 
think you can
guarantee that it won't mess up the masks on the JRE threads if the PNG is bad. 
And I'm
also not sure you want to remove error handling from the library either.
So a HIGHLY VISIBLE DO NOT REMOVE comment might be the best you can do here.
I have a better idea:

png_default_error is the only place where png_longjmp is called. We could call 
png_set_error_fn to set up our own error handler (for Mac only), compile with 
PNG_SETJMP_SUPPORTED unset so it doesn't pull in setjmp/longjmp and our own 
implementation of the error handler would call _longjmp, which would jump back 
to where we call setjmp currently.
Ok, I figured out what's going on. It's not quite intuitive...

png_jmpbuf is a macro defined in png.h, this calls png_set_longjmp_fn with 
longjmp, which is why I was seeing references to longjmp in the object file. 
That's what was throwing me off as it seems like it should only be getting the 
jmp_buf ptr stored in the png_ptr. I guess the intention was that 
setjmp/longjmp was optional, if you don't call setjmp then it just abort()s.


I changed splashscreen_png.c to:
#ifdef __APPLE__
    if (_setjmp(png_set_longjmp_fn(png_ptr, _longjmp, sizeof(jmp_buf)))) {
#else
    if (setjmp(png_jmpbuf(png_ptr))) {
#endif

and it calls _longjmp instead. I verified this works by changing the macro to 
set png_longjmp to exit() and without the above change it does indeed exit 
prematurely with a bad png, with the change it reports the error but continues 
to load the application as would be expected.

pngread.o still has a symbol table entry for _longjmp instead of __longjmp, but 
it's benign since we're ultimately forcing it to use the correct function. So 
I've left libpng completely unchanged.


With the change and using a bad png for splashscreen, I was able to get a stack 
trace once the application was running. Without the change to 
splashscreen_png.c, jstack was unable to connect to the process. So 
splashscreen absolutely can interfere with the signal handling.


Updated webrev:
http://cr.openjdk.java.net/~ddehaven/8026385/jdk.1/

I can look into writing a regression test for this. It might not be trivial 
though since we're dealing with signal handlers, and if timing is a factor the 
test may not be reliable.

-DrD-


Reply via email to