Staffan, Looks good for me!
-Dmitry On 2014-03-27 11:55, Staffan Larsen wrote: > Here is an updated webrev which incorporates Dmitry’s feedback: > > http://cr.openjdk.java.net/~sla/8038296/webrev.01/ > > Thanks, > /Staffan > > On 25 mar 2014, at 19:36, Dmitry Samersoff <[email protected]> > wrote: > >> Staffan, >> >>> Yes, that will find problems when trying to convert something like >>> ‘bla’. It will not capture the case where the input string is a too >>> large (or small) value (value < LONG_MIN or > LONG_MAX). To capture >>> both cases it looks like we need something like: >>> errno = 0; >>> char* end; >>> int probe_typess = (int) strtol(probe, &end, 10); >>> if (end == probe || errno) { >>> return JNI_ERR; >>> } >> >> As probe_typess is positive and you are converting long to int >> It's be better to check value boundaries explicitly: >> >> char* end; >> long ptl = strtol(probe, &end, 10); >> if (end == probe || ptl < 0 || ptl > MAX_INT) { >> return JNI_ERR; >> } >> >> int probe_typess = (int) ptl; >> >> >> -Dmitry >> >> >> On 2014-03-25 17:35, Staffan Larsen wrote: >>> >>> On 25 mar 2014, at 13:46, Dmitry Samersoff >>> <[email protected]> wrote: >>> >>>> Staffan, >>>> >>>> strtol() sets errno in two cases - >>>> >>>> ERANGE if supplied value is less then LONG_MIN or greater than >>>> LONG_MAX EINVAL if supplied base is not supported. >>>> >>>> if you pass probe == 'bla', strtol just return 0 and errno will not >>>> be set. So I'm not sure that check for errno has any value here. >>>> >>>> One of possible way to check that supplied value is convertible to >>>> long is >>>> >>>> char *eptr = probe; strtol(probe, (char **)&eptr, 10); if (eptr == >>>> probe) { // we can't convert supplied value return JNI_ERR; } >>> >>> Yes, that will find problems when trying to convert something like >>> ‘bla’. It will not capture the case where the input string is a too >>> large (or small) value (value < LONG_MIN or > LONG_MAX). To capture >>> both cases it looks like we need something like: >>> >>> errno = 0; char* end; int probe_typess = (int) strtol(probe, &end, >>> 10); if (end == probe || errno) { return JNI_ERR; } >>> >>> >>> /Staffan >>> >>>> >>>> -Dmitry >>>> >>>> >>>> On 2014-03-25 11:31, Staffan Larsen wrote: >>>>> attachListener_solaris.cpp calls atoi() and then checks errno to >>>>> see if any errors occurred. The problem is that atoi() does not >>>>> set errno, so some old value of errno is checked which sometimes >>>>> causes the function to fail. >>>>> >>>>> The fix is to replace atoi() with strtol() which does set errno. >>>>> But errno is only set if an error occurred and not set to 0 in >>>>> case of success. Thus, I set errno to 0 before calling strtol() >>>>> and check the value afterwards. >>>>> >>>>> Verified with a JPRT run. >>>>> >>>>> webrev: http://cr.openjdk.java.net/~sla/8038296/webrev.00/ bug: >>>>> https://bugs.openjdk.java.net/browse/JDK-8038296 >>>>> >>>>> Thanks, /Staffan >>>>> >>>> >>>> >>>> -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, >>>> Russia * I would love to change the world, but they won't give me >>>> the sources. >>> >> >> >> -- >> Dmitry Samersoff >> Oracle Java development team, Saint Petersburg, Russia >> * I would love to change the world, but they won't give me the sources. > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
