Re: [PATCH 2/2] netfilter: xt_condition: change the value from boolean to u32

[Luciano Coelho <luciano.coelho@nokia.com>]

On Fri, 2010-08-06 at 10:52 +0200, ext Jan Engelhardt wrote:
> On Friday 2010-08-06 10:00, Luciano Coelho wrote:
> >> >+	buf[length - 1] = '\0';
> >> >+
> >> >+	if (strict_strtoull(buf, 0, &value) != 0)
> >> >+		return -EINVAL;
> >> >+
> >> >+	if (value > (u32) value)
> >> >+		return -EINVAL;
> >> 
> >> Is it possible to use just strict_strtoul?
> >
> >Not easily.  I found that there is a bug in strtoul (and strtoull for
> >that matter) that causes the long to overflow if there are valid digits
> >after the maximum possible digits for the base.  For example if you try
> >to strtoul 0xfffffffff (with 9 f's) the strtoul will overflow and come
> >up with a bogus result.
> 
> I see. Strange that no one has adressed this yet - I mean, writing
> a just-too-large value into a procfs/sysfs file and thus effectively
> causing a bogus value to be actually written isn't quite so thrilling
> as things go haywire.

Yes, I was really surprised to see this happening when I was testing the
limits.  And I was even more surprised when I checked the strtoull code
and saw that it is broken.


> >I can't easily truncate the string to avoid
> >this problem, because with decimal or octal, the same valid value would
> >take more spaces.  I could do some magic here, checking whether it's a
> >hex, dec or oct and truncate appropriately, but that would be very ugly.
> >
> >So the simplest way I came up with was to use strtoull and return
> >-EINVAL if the value exceeds 32 bits. ;)
> 
> If I read strtoul(3) right, ERANGE is used for "out of range".

Yes, libc's strtoul returns ERANGE in that case.  strict_strtoul() in
the kernel code doesn't.  I'll change my code to return -ERANGE here
too, for consistency.


> >> Since the condition value (cdmark) was thought of an nfmark-style thing, 
> >> would it perhaps make sense to model it after it
> >> 
> >> 	return (var->value & ~info->mask) ^ info->value;
> >> 
> >> Other opinions?
> >
> >I think it's nicer to have it as a normal equals here for now and then
> >extend the match with more operations.  We can later add, for example,
> >an --and option to the condition match in order to do other kinds of
> >binary operations.  It would be more flexible this way because we could
> >use several different types of comparisons, wouldn't it? And in the
> >target we could have several different types of operations.
> 
> Indeed.


-- 
Cheers,
Luca.