[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 24 Jul 2019 08:53:52 +0200
From: Rasmus Villemoes <linux@...musvillemoes.dk>
To: Joe Perches <joe@...ches.com>, Rasmus Villemoes
<linux@...musvillemoes.dk>, Linus Torvalds <torvalds@...ux-foundation.org>,
linux-kernel@...r.kernel.org
Cc: Jonathan Corbet <corbet@....net>, Stephen Kitt <steve@....org>,
Kees Cook <keescook@...omium.org>, Nitin Gote <nitin.r.gote@...el.com>,
jannh@...gle.com, kernel-hardening@...ts.openwall.com,
Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms
On 23/07/2019 17.39, Joe Perches wrote:
> On Tue, 2019-07-23 at 16:37 +0200, Rasmus Villemoes wrote:
>> On 23/07/2019 15.51, Joe Perches wrote:
>>>
>>> These mechanisms verify that the dest argument is an array of
>>> char or other compatible types like u8 or s8 or equivalent.
>> Sorry, but "compatible types" has a very specific meaning in C, so
>> please don't use that word.
>
> I think you are being overly pedantic here but
> what wording do you actually suggest?
I'd just not support anything other than char[], but if you want,
perhaps say "related types", or some other informal description.
>> And yes, the kernel disables -Wpointer-sign,
>> so passing an u8* or s8* when strscpy() expects a char* is silently
>> accepted, but does such code exist?
>
> u8 definitely, s8 I'm not sure.
Example (i.e. of someone passing an u8* as destination to some string
copy/formatting function)? I believe you, I'd just like to see the context.
> I don't find via grep a use of s8 foo[] = "bar";
> or "signed char foo[] = "bar";
>
> I don't think it bad to allow it.
Your patch.
>> count is just as bad as size in terms of "the expression src might
>> contain that identifier". But there's actually no reason to even declare
>> a local variable, just use ARRAY_SIZE() directly as the third argument
>> to strscpy().
>
> I don't care about that myself.
> It's a macro local identifier and shadowing in a macro
> is common. I'm not a big fan of useless underscores.
shadowing is not the problem. The identifier "count" appearing in one of
the "dest" or "src" expressions is. For something that's supposed to
help eliminate bugs, such a hidden footgun seems to be a silly thing to
include. No need for some hideous triple-underscore variable, just make
the whole thing
BUILD_BUG_ON(!__same_type())
strscpy(dst, src, ARRAY_SIZE(dst))
Rasmus
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
