Thanks Serguei, I don’t think it is necessary to initialize ‘end’ since strtol will always set it.
I still need an official Reviewer for this change. Thanks, /Staffan On 28 mar 2014, at 07:21, [email protected] wrote: > On 3/27/14 12:55 AM, Staffan Larsen wrote: >> Here is an updated webrev which incorporates Dmitry’s feedback: >> >> http://cr.openjdk.java.net/~sla/8038296/webrev.01/ > > > It looks good. > The only suggestion is to initialize the 'end': > char* end; > > Not sure what value is better to use for initialization. > Probably, end = probe would work Ok. > > Thanks, > Serguei >> >> 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. >
