On Mon, Jan 28, 2008 at 04:46:06PM -0500, Chuck Lever wrote:
> Hi Bruce-
>
> On Jan 28, 2008, at 3:15 PM, J. Bruce Fields wrote:
>> On Mon, Jan 28, 2008 at 01:00:47PM -0500, Chuck Lever wrote:
>>> Changing nbytes to an unsigned introduces a mixed-sign comparison:
>>>
>>> int avail = (char*)argp->end - (char*)argp->p;
>>> __be32 *p;
>>> if (avail + argp->pagelen < nbytes) <<<<<<
>>>
>>> "avail" and "argp->pagelen" are signed ints.
>>
>> OK. Is the result incorrect?
>
> Whether it is or it isn't, I'm not comfortable with introducing a
> mixed-sign comparison for no reason. It's not a correctness issue, it's
> a maintainability issue.
I understand, I just wanted to make sure.
(As a general rule, it would be helpful to me if, both in comments like
this and on any patches, you'd clearly state whether or not you've found
a run-time bug.)
>
>> (Any objections to just making "avail" unsigned?)
>
> If we can guarantee that argp->end is always larger than argp->p... then
> there's no need for "avail" to be a signed int. However:
OK.
>
>>> return NULL;
>>> if (avail + PAGE_SIZE < nbytes) /* need more than a page !!
>>> */
>>> return NULL;
>>>
>>> PAGE_SIZE is unsigned long (defined as "1UL << PAGE_SHIFT"), so
>>> changing
>>> nbytes may have additional unintended consequences.
>
> ...and since that wasn't noticed or addressed by this patch, it suggests
> that there may be other issues that the patch author didn't address and
> that reviewers didn't catch.
The change in behavior of these comparisons was exactly the point of
the patch:
"The length "nbytes" passed into read_buf should never be
negative, but we check only for too-large values of "nbytes",
not for too-small values. Make nbytes unsigned, so it's clear
that the former tests are sufficient."
Was my comment there unclear, or am I missing some other problem?
--b.
-
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
