I've got a fix that I'm testing. I'll forward once I'm happy Sent from my iPhone On 20/06/2010, at 12:01, FUJITA Tomonori <fujita.tomonori at lab.ntt.co.jp> wrote: > On Thu, 17 Jun 2010 16:19:43 +0300 (IDT) > Alexander Nezhinsky <alexandern at voltaire.com> wrote: > >> When a numerical command line param value is out of range or >> contains garbage >> (both tgtd and tgtadm), this remains unchecked in many cases. >> In some of them 0 value will be used, which may lead to unpredicted >> or >> "silently" faulty behavior. >> >> This fix defines a new function str_to_uit64() which uses strtoull >> () and >> detects its errors properly, by checking 1) errno (range errors) and >> 2) comparing endptr to the original argument (nun-numerical garbage); >> must check both. >> >> str_to_uit64() is declared static twice, in tgtd.c and tgtadm.c, as >> tgtadm does not uses util.c, where the natural place of this function >> would be otherwise. >> >> It returns the converted integer value directly, while error is >> returned >> through a pointer argument. Although the opposite arrangement may >> seem >> intuitive, this one allows free casting of the returned value to >> various >> types. Return type uint64 is choosen to cover all possible cases. >> min and max arguments support range checking. To use the natural >> integer >> ranges, use ULLONG_MAX, ULONG_MAX, USHRT_MAX etc. >> >> Signed-off-by: Alexander Nezhinsky <alexandern at voltaire.com> > > I'm for the strict checking however two comments. > > - casting a returned value looks ugly. > > - duplicating the same function isn't nice. > > > How about something like this? > > diff --git a/usr/tgtadm.c b/usr/tgtadm.c > index 28ed754..2282c86 100644 > --- a/usr/tgtadm.c > +++ b/usr/tgtadm.c > @@ -471,8 +471,8 @@ int main(int argc, char **argv) > mode = str_to_mode(optarg); > break; > case 't': > - tid = strtol(optarg, NULL, 10); > - if (errno == EINVAL) { > + tid = str_to_val(optarg, int, 0, INT_MAX, &rc); > + if (rc == EINVAL) { > eprintf("invalid tid '%s'\n", optarg); > exit(EINVAL); > } > diff --git a/usr/util.h b/usr/util.h > index 6e1fd6f..fc406b1 100644 > --- a/usr/util.h > +++ b/usr/util.h > @@ -156,4 +156,17 @@ struct signalfd_siginfo { > }; > #endif > > +#define str_to_val(str, type, min, max, err_ptr) \ > +({ \ > + char *ptr; \ > + int ret = 0; \ > + typeof(type) v = strtoull(str, &ptr, 0); \ > + if (errno || ptr == str) \ > + ret = EINVAL; \ > + if (v < min|| v > max) \ > + ret = ERANGE; \ > + *err_ptr = ret; \ > + v; \ > +}) \ > + > #endif > -- > To unsubscribe from this list: send the line "unsubscribe stgt" in > the body of a message to majordomo at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe stgt" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html |