Could I have an official Review of this change? Thanks, /Staffan
On 3 apr 2014, at 11:31, Staffan Larsen <[email protected]> wrote: > 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. >> >
