On 1 October 2013 15:47, Christopher Schultz <ch...@christopherschultz.net> wrote: > Mladen, > > On 10/1/13 10:39 AM, Mladen Turk wrote: >> On 10/01/2013 04:15 PM, Christopher Schultz wrote: >>> Mladen and Rainer, >>>>> >>>>> -1. You are just hiding the reason for crash. >>> >>> I disagree: the reason for the crash can still be reported. It just >>> won't be reported with a JVM crash: instead, there will be an exception. >> >> I disagree on your disagreement :) > > ;) > > Just to be sure: I'm not arguing against fixing the Java code, or > providing checks in the Java code to avoid SIGSEGVs. I'm just saying > that native code should be as safe as reasonably possible. NULL-checks > are cheap, too, so I don't think there's any reason to avoid them in > native code. > >> This can be easily done in Java with much cheaper computing cost. > > Agreed. Feel free to avoid native calls if you know the data is bad. But > we can't force people to use specific versions of Tomcat and tcnative > together (though obviously, newer versions of Tomcat can refuse to load > older versions of tcnative). Someone pointed out in Bugzilla (though it > mostly fell on deaf ears) that tcnative isn't used exclusively for > Tomcat: some folks are using it as a convenient library to access APR > and other native capabilities in a Java wrapper. > >> If you suspect the data fed to native call can be faulty, well check it >> and throw java exception instead calling native, and distinguishing >> between valid >> and error return values from native and then still have a code which will >> pass/throw. > > I was suggesting throwing an exception directly from native code, not > trying to return a status code and then throwing from Java-land. > >> I know it requires a different thinking then average library, >> but it's not an average library. It's wrapper for APR and APR expects you >> provide valid data. >> >> Even checking in native won't solve crashes. You can fed a long (pointer) >> which is in zombie state (closed but not null) and you'll have a memory >> location which will pass null check but can still crash or even corrupt >> other memory location if it was reused by another alloc. > > Sure, but a NULL-check (whether in Java or C) isn't going to catch that > either way, and nothing we've been discussing here will have any bearing > on that situation: it will still fail, it will likely still crash or > behave strangely and ... > >> Much harder to track down. > > ... it will be hard to track down. Adding NULL-checks will not > complicate any of that. It will not mask any of that. It will make it no > more difficult to track-down or debug. It will only prevent the JVM > going down when a pointer ends up being NULL, which is the only reason > I'm suggesting it. > > Given the two -1s and lack of any +1s on this suggestion, I won't be > committing anything, but I still think it's a worthwhile endeavor > because it's so easy to do and protects us from such disastrous results.
+1 (non-binding!) I agree with Mladen that relying on tcnative to do all the validation is wrong; the Java code should be written so as to only pass valid parameters. But mistakes happen, and not all applications using tcnative are written by Tomcat developers who should be familiar with the calling conventions. The JVM throws an exception if a Java application uses an invalid array index or tries to read past the end of an iterator. That does not mean that Java applications should use these exceptions as part of standard coding; of course the application should try to use valid parameters. The point is that such exceptions make debugging much easier. If Java crashed whenever an invalid parameter was provided, it would not be very popular with developers. If a Java application succeeds in crashing the JVM, then IMO the JVM has a bug. I believe that all native code should strive to behave the same way. > -chris > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
