Prev: [RFC PATCH 0/1] Driver for ami305 magnetometer
Next: -next: Nov 12 - kernel BUG at kernel/sched.c:7359!
From: Casey Schaufler on 14 Nov 2009 00:20
Joe Perches wrote:
> On Sat, 2009-11-14 at 03:44 +0000, David Wagner wrote:
>
>> I personally don't find
>> strncmp(foo, "constant", sizeof("constant")) // first snippet
>> to be more readable, auditable, or obviously correct than
>> strcmp(foo, "constant"). // second snippet
>> Is there a technical basis for arguing that the first
>> snippet is better than the second snippet?
>>
>
> I don't think there is.
>

And you're exactly correct. Now please go convince all the whingers
who think that even though because their tool found a "bad" thing
there is nothing to worry about. But that's beside the point. There
really is no point here. This whole discussion is around a gratuitous
change that has no net effect on the behavior of the system. Unless
you are talking about the original change proposal, which would have
broken certain cases.

I am advocating that the code be left as is. It works fine (for what it
is intended to do, of course) and the "corrected" change is just plain
unnecessary. It is no clearer and no less clear than the original. Leave
it alone unless there is a good reason to change it. What, are y'all
getting paid by the patch or something?


> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo(a)vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Joe Perches on 14 Nov 2009 00:30
On Fri, 2009-11-13 at 21:12 -0800, Casey Schaufler wrote:
> Joe Perches wrote:
> > On Sat, 2009-11-14 at 03:44 +0000, David Wagner wrote:
> >> I personally don't find
> >> strncmp(foo, "constant", sizeof("constant")) // first snippet
> >> to be more readable, auditable, or obviously correct than
> >> strcmp(foo, "constant"). // second snippet
> >> Is there a technical basis for arguing that the first
> >> snippet is better than the second snippet?
> > I don't think there is.
> And you're exactly correct.
> This whole discussion is around a gratuitous
> change that has no net effect on the behavior of the system.

It has relatively little or no effect on a
running system, but does effect code
readability.

> I am advocating that the code be left as is.

I assert that code should be made as readable
as possible and that the code used fit the
reader's expectations.

strcmp(foo, "BAR") is natural.
strncmp(foo, "BAR", sizeof("BAR")) is unnatural
and should not be used.

cheers, Joe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Casey Schaufler on 14 Nov 2009 02:30
Joe Perches wrote:
> On Fri, 2009-11-13 at 21:12 -0800, Casey Schaufler wrote:
>
>> Joe Perches wrote:
>>
>>> On Sat, 2009-11-14 at 03:44 +0000, David Wagner wrote:
>>>
>>>> I personally don't find
>>>> strncmp(foo, "constant", sizeof("constant")) // first snippet
>>>> to be more readable, auditable, or obviously correct than
>>>> strcmp(foo, "constant"). // second snippet
>>>> Is there a technical basis for arguing that the first
>>>> snippet is better than the second snippet?
>>>>
>>> I don't think there is.
>>>
>> And you're exactly correct.
>> This whole discussion is around a gratuitous
>> change that has no net effect on the behavior of the system.
>>
>
> It has relatively little or no effect on a
> running system, but does effect code
> readability.
>
>
>> I am advocating that the code be left as is.
>>
>
> I assert that code should be made as readable
> as possible and that the code used fit the
> reader's expectations.
>
> strcmp(foo, "BAR") is natural.
> strncmp(foo, "BAR", sizeof("BAR")) is unnatural
> and should not be used.
>
>

Oh good gravy. I've been writing C code since the 1970's and
have seen enough "unnatural" code to make most people think that
PASCAL was a good idea. This is not unnatural code. This is an
argument over which side of the head of the pin the odd angel
should dance on. Give it up. You're advocating a gratuitous
change. Can't y'all go find some questionable casts to expunge?
That might actually be useful.

> cheers, Joe
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo(a)vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Julia Lawall on 14 Nov 2009 10:30
On Fri, 13 Nov 2009, Valdis.Kletnieks(a)vt.edu wrote:

> On Fri, 13 Nov 2009 22:26:20 +0100, Julia Lawall said:
> > On Fri, 13 Nov 2009, Valdis.Kletnieks(a)vt.edu wrote:
> > > Julia, is there a way to use coccinelle to detect unsafe changes like that? Or
> > > is expressing those semantics too difficult?
> >
> > Could you give a concrete example of something that would be a problem?
> > If something like alias analysis is required, to know what strings a
> > variable might be bound to, that might be difficult. Coccinelle works
> > better when there is some concrete codeto match against.
>
> Here's a concrete example of how a previously audited strcmp() can go bad...
>
> struct foo {
> char[16] a; /* old code allows 15 chars and 1 more for the \0 */
> int b;
> int c;
> }
>
> bzero(foo,sizeof(foo));
>
> Now code can pretty safely mess with the first 15 bytes of foo->a and
> we know we're OK if we call strcmp(foo->a,....) because that bzero()
> nuked a[15] for us. It's safe to strncpy(foo->a,bar,15); and not worry
> about the fact that if bar is 15 chars long, a trailing \0 won't be put in.
>
> Now somebody comes along and does:
>
> struct foo {
> char *a; /* we need more than 15 chars for some oddball hardware */
> int b;
> int c;
> }
>
> bzero(foo,sizeof(foo));
> foo->a = kmalloc(32); /* whoops should have been kzmalloc */
>
> Now suddenly, strncpy(foo->a,bar,31); *isn't* safe....
>
> (Yes, I know there's plenty of blame to go around in this example - the failure
> to use kzmalloc, the use of strncpy() without an explicit \0 being assigned
> someplace, the use of strcmp() rather than strncmp()... But our tendency to
> intentionally omit several steps of this to produce more efficient code means
> it's easier to shoot ourselves in the foot...)

Thanks for the example. Coccinelle only finds patterns of code in one
version, while this would require considering two versions at once. Such
a thing could be interesting though.

julia
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Raja R Harinath on 15 Nov 2009 03:40
Hi,

Casey Schaufler <casey(a)schaufler-ca.com> writes:

> Joe Perches wrote:
[snip]
>> I assert that code should be made as readable
>> as possible and that the code used fit the
>> reader's expectations.
>>
>> strcmp(foo, "BAR") is natural.
>> strncmp(foo, "BAR", sizeof("BAR")) is unnatural
>> and should not be used.
>
> Oh good gravy. I've been writing C code since the 1970's and
> have seen enough "unnatural" code to make most people think that
> PASCAL was a good idea. This is not unnatural code. This is an
> argument over which side of the head of the pin the odd angel
> should dance on. Give it up. You're advocating a gratuitous
> change. Can't y'all go find some questionable casts to expunge?
> That might actually be useful.

I think the point is that

strncmp(foo, "BAR", sizeof("BAR"))

is exceedingly similar to

strncmp(foo, "BAR", strlen("BAR"))

which mean different things. The point of this series was the suspicion
that people who intended the "strlen" variant might have used the
"sizeof" variant.

And, since this confusion exists, it is probably better to use two
canonical forms for the two different meanings

strcmp(foo, "BAR")
strncmp(foo, "BAR", strlen("BAR"))

and avoid other equivalent formulations.

- Hari

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  |  Next  |  Last
Pages: 1 2 3 4 5
Prev: [RFC PATCH 0/1] Driver for ami305 magnetometer
Next: -next: Nov 12 - kernel BUG at kernel/sched.c:7359!