lkml.org 
[lkml]   [2019]   [Sep]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH V2 1/2] string: Add stracpy and stracpy_pad mechanisms
Le 26/09/2019 09:29, Rasmus Villemoes a écrit :
> On 26/09/2019 02.01, Stephen Kitt wrote:
>> Le 25/09/2019 23:50, Andrew Morton a écrit :
>>> On Tue, 23 Jul 2019 06:51:36 -0700 Joe Perches <joe@perches.com>
>>> wrote:
>>>
>>>> Several uses of strlcpy and strscpy have had defects because the
>>>> last argument of each function is misused or typoed.
>>>>
>>>> Add macro mechanisms to avoid this defect.
>>>>
>>>> stracpy (copy a string to a string array) must have a string
>>>> array as the first argument (dest) and uses sizeof(dest) as the
>>>> count of bytes to copy.
>>>>
>>>> These mechanisms verify that the dest argument is an array of
>>>> char or other compatible types like u8 or s8 or equivalent.
>>>>
>>>> A BUILD_BUG is emitted when the type of dest is not compatible.
>>>>
>>>
>>> I'm still reluctant to merge this because we don't have code in -next
>>> which *uses* it.  You did have a patch for that against v1, I
>>> believe?
>>> Please dust it off and send it along?
>>
>> Joe had a Coccinelle script to mass-convert strlcpy and strscpy.
>> Here's a different patch which converts some of ALSA's strcpy calls to
>> stracpy:
>
> Please don't. At least not for the cases where the source is a string
> literal - that just gives worse code generation (because gcc doesn't
> know anything about strscpy or strlcpy), and while a run-time (silent)
> truncation is better than a run-time buffer overflow, wouldn't it be
> even better with a build time error?

Yes, that was the plan once Joe's patch gets merged (if it does), and my
patch was only an example of using stracpy, as a step on the road. I was
intending to follow up with a patch converting stracpy to something like
https://www.openwall.com/lists/kernel-hardening/2019/07/06/14

__FORTIFY_INLINE ssize_t strscpy(char *dest, const char *src, size_t
count)
{
size_t dest_size = __builtin_object_size(dest, 0);
size_t src_size = __builtin_object_size(src, 0);
if (__builtin_constant_p(count) &&
__builtin_constant_p(src_size) &&
__builtin_constant_p(dest_size) &&
src_size <= count &&
src_size <= dest_size &&
src[src_size - 1] == '\0') {
strcpy(dest, src);
return src_size - 1;
} else {
return __strscpy(dest, src, count);
}
}

which, as a macro, would become

#define stracpy(dest, src) \
({ \
size_t count = ARRAY_SIZE(dest); \
size_t dest_size = __builtin_object_size(dest, 0); \
size_t src_size = __builtin_object_size(src, 0); \
BUILD_BUG_ON(!(__same_type(dest, char[]) || \
__same_type(dest, unsigned char[]) || \
__same_type(dest, signed char[]))); \
\
(__builtin_constant_p(count) && \
__builtin_constant_p(src_size) && \
__builtin_constant_p(dest_size) && \
src_size <= count && \
src_size <= dest_size && \
src[src_size - 1] == '\0') ? \
(((size_t) strcpy(dest, src)) & 0) + src_size - 1 \
: \
strscpy(dest, src, count); \
})

and both of these get optimised to movs when copying a constant string
which fits in the target.

I was going at this from the angle of improving the existing APIs and
their resulting code. But I like your approach of failing at compile
time.

Perhaps we could do both ;-).

Regards,

Stephen

\
 
 \ /
  Last update: 2019-09-26 11:05    [W:0.030 / U:36.624 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site