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@xxxxxxxxxxx> 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? Something like /** static_strcpy - run-time version of static initialization * * @d: destination array, must be array of char of known size * @s: source, must be a string literal * * This is a simple wrapper for strcpy(d, s), which checks at build-time that the strcpy() won't overflow. In most cases (for short strings), gcc won't even emit a call to strcpy but rather just do a few immediate stores into the destination, e.g. 0: c7 07 66 6f 6f 00 movl $0x6f6f66,(%rdi) * for strcpy(d->name, "foo"). */ #define static_strcpy(d, s) ({ \ static_assert(__same_type(d, char[]), "destination must be char array"); \ static_assert(__same_type(s, const char[], "source must be a string literal"); \ static_assert(sizeof(d) >= sizeof("" s ""), "source does not fit in destination"); \ strcpy(d, s); \ }) The "" s "" trick guarantees that s is a string literal - it probably doesn't give a very nice error message, but that's why I added the redundant type comparison to a const char[] which should hopefully give a better clue. Rasmus PS: Yes, we already have a fortified strcpy(), but for some reason it doesn't catch the common case where we're populating a char[] member of some struct. So this diff --git a/lib/vsprintf.c b/lib/vsprintf.c index e78017a3e1bd..3b0c5ae9f49e 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -3420,3 +3420,14 @@ int sscanf(const char *buf, const char *fmt, ...) return i; } EXPORT_SYMBOL(sscanf); + +struct s { char name[4]; }; +char buf[4]; +void f3(void) +{ + strcpy(buf, "long"); +} +void f4(struct s *s) +{ + strcpy(s->name, "long"); +} with CONFIG_FORTIFY_SOURCE=y complains about f3(), but f4() is just fine... PPS: A quick test of static_strcpy(): // ss.c #include <errno.h> #include <string.h> #include <assert.h> #define __same_type(x, y) __builtin_types_compatible_p(typeof(x), typeof(y)) #define static_strcpy(d, s) ({ \ static_assert(__same_type(d, char[]), "destination must be char array"); \ static_assert(__same_type(s, const char[]), "source must be a string literal"); \ static_assert(sizeof(d) >= sizeof("" s ""), "source does not fit in destination"); \ strcpy(d, s); \ }) struct s { char name[4]; }; void f0(struct s *s) { static_strcpy(s->name, "foo"); } #if 1 void f1(struct s *s) { static_strcpy(s->name, strerror(EIO)); } void f2(struct s *s) { static_strcpy(s->name, "long"); } void f3(char *d) { static_strcpy(d, "foo"); } void f4(char *d) { static_strcpy(d, strerror(EIO)); } #endif // gcc -Wall -O2 -o ss.o -c ss.c In file included from ss.c:3:0: ss.c: In function ‘f1’: ss.c:9:3: error: static assertion failed: "source must be a string literal" static_assert(__same_type(s, const char[]), "source must be a string literal"); \ ^ ss.c:18:24: note: in expansion of macro ‘static_strcpy’ void f1(struct s *s) { static_strcpy(s->name, strerror(EIO)); } ^~~~~~~~~~~~~ ss.c:18:47: error: expected ‘)’ before ‘strerror’ void f1(struct s *s) { static_strcpy(s->name, strerror(EIO)); } ^ ss.c:10:40: note: in definition of macro ‘static_strcpy’ static_assert(sizeof(d) >= sizeof("" s ""), "source does not fit in destination"); \ ^ In file included from ss.c:3:0: ss.c: In function ‘f2’: ss.c:10:3: error: static assertion failed: "source does not fit in destination" static_assert(sizeof(d) >= sizeof("" s ""), "source does not fit in destination"); \ ^ ss.c:19:24: note: in expansion of macro ‘static_strcpy’ void f2(struct s *s) { static_strcpy(s->name, "long"); } ^~~~~~~~~~~~~ ss.c: In function ‘f3’: ss.c:8:3: error: static assertion failed: "destination must be char array" static_assert(__same_type(d, char[]), "destination must be char array"); \ ^ ss.c:20:20: note: in expansion of macro ‘static_strcpy’ void f3(char *d) { static_strcpy(d, "foo"); } ^~~~~~~~~~~~~ ss.c: In function ‘f4’: ss.c:8:3: error: static assertion failed: "destination must be char array" static_assert(__same_type(d, char[]), "destination must be char array"); \ ^ ss.c:21:20: note: in expansion of macro ‘static_strcpy’ void f4(char *d) { static_strcpy(d, strerror(EIO)); } ^~~~~~~~~~~~~ ss.c:9:3: error: static assertion failed: "source must be a string literal" static_assert(__same_type(s, const char[]), "source must be a string literal"); \ ^ ss.c:21:20: note: in expansion of macro ‘static_strcpy’ void f4(char *d) { static_strcpy(d, strerror(EIO)); } ^~~~~~~~~~~~~ ss.c:21:37: error: expected ‘)’ before ‘strerror’ void f4(char *d) { static_strcpy(d, strerror(EIO)); } ^ ss.c:10:40: note: in definition of macro ‘static_strcpy’ static_assert(sizeof(d) >= sizeof("" s ""), "source does not fit in destination"); \ ^