| Message ID | 20190709154806.26363-1-nitin.r.gote@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] Added warnings in checkpatch.pl script to : | expand |
On Tue, 2019-07-09 at 21:18 +0530, NitinGote wrote: > From: Nitin Gote <nitin.r.gote@intel.com> > > 1. Deprecate strcpy() in favor of strscpy(). > 2. Deprecate strlcpy() in favor of strscpy(). > 3. Deprecate strncpy() in favor of strscpy() or strscpy_pad(). > > Updated strncpy() section in Documentation/process/deprecated.rst > to cover strscpy_pad() case. Please slow down your patch submission rate for this instance and respond appropriately to the comments you've been given. This stuff is not critical bug fixing. The subject could be something like: Subject: [PATCH v#] Documentation/checkpatch: Prefer strscpy over strcpy/strlcpy > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl [] > @@ -605,6 +605,20 @@ foreach my $entry (keys %deprecated_apis) { > } > $deprecated_apis_search = "(?:${deprecated_apis_search})"; > > +our %deprecated_string_apis = ( > + "strcpy" => "strscpy", > + "strlcpy" => "strscpy", > + "strncpy" => "strscpy, strscpy_pad or for non-NUL-terminated strings, strncpy() can still be used, but destinations should be marked with the __nonstring", 'the' is not necessary. There could likely also be a strscat created for strcat, strlcat and strncat. btw: There were several defects in the kernel for misuses of strlcpy. Did you or anyone else have an opinion on stracpy to avoid duplicating the first argument in a sizeof()? strlcpy(foo, bar, sizeof(foo)) to stracpy(foo, bar) where foo must be char array compatible ? https://lore.kernel.org/lkml/d1524130f91d7cfd61bc736623409693d2895f57.camel@perches.com/
> -----Original Message----- > From: Joe Perches [mailto:joe@perches.com] > Sent: Tuesday, July 9, 2019 9:40 PM > To: Gote, Nitin R <nitin.r.gote@intel.com>; corbet@lwn.net > Cc: akpm@linux-foundation.org; apw@canonical.com; > keescook@chromium.org; linux-doc@vger.kernel.org; linux- > kernel@vger.kernel.org; kernel-hardening@lists.openwall.com > Subject: Re: [PATCH v4] Added warnings in checkpatch.pl script to : > > On Tue, 2019-07-09 at 21:18 +0530, NitinGote wrote: > > From: Nitin Gote <nitin.r.gote@intel.com> > > > > 1. Deprecate strcpy() in favor of strscpy(). > > 2. Deprecate strlcpy() in favor of strscpy(). > > 3. Deprecate strncpy() in favor of strscpy() or strscpy_pad(). > > > > Updated strncpy() section in Documentation/process/deprecated.rst > > to cover strscpy_pad() case. > > Please slow down your patch submission rate for this instance and respond > appropriately to the comments you've been given. Sure, I will explore this things more. And sorry, I missed to incorporate one comment. I will take care of such things. > > This stuff is not critical bug fixing. > Noted. > The subject could be something like: > > Subject: [PATCH v#] Documentation/checkpatch: Prefer strscpy over > strcpy/strlcpy > How about this : Subject: [PATCH v#] Doc/checkpatch: Prefer strscpy/strscpy_pad over strcpy/strlcpy/strncpy > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > [] > > @@ -605,6 +605,20 @@ foreach my $entry (keys %deprecated_apis) { } > > $deprecated_apis_search = "(?:${deprecated_apis_search})"; > > > > +our %deprecated_string_apis = ( > > + "strcpy" => "strscpy", > > + "strlcpy" => "strscpy", > > + "strncpy" => "strscpy, strscpy_pad or > for non-NUL-terminated strings, strncpy() can still be used, but destinations > should be marked with the __nonstring", > > 'the' is not necessary. Noted. > > There could likely also be a strscat created for strcat, strlcat and strncat. > I have not found reference for strscat in kernel. Could you please give any reference for strscat ? > btw: > > There were several defects in the kernel for misuses of strlcpy. > > Did you or anyone else have an opinion on stracpy to avoid duplicating the > first argument in a sizeof()? > > strlcpy(foo, bar, sizeof(foo)) > to > stracpy(foo, bar) > > where foo must be char array compatible ? > > https://lore.kernel.org/lkml/d1524130f91d7cfd61bc736623409693d2895f57. > camel@perches.com/ > > As I understood, your trying to give new interface like stracpy(), to avoid duplication of first argument in a sizeof(), we can also make it more robust for users by adding check or warn in checkpatch.pl to prefer stracpy(). Did you or anyone has opinion on this ? Thanks, Nitin Gote
diff --git a/Documentation/process/deprecated.rst b/Documentation/process/deprecated.rst index 49e0f64a3427..0fb37ebe3ad9 100644 --- a/Documentation/process/deprecated.rst +++ b/Documentation/process/deprecated.rst @@ -93,9 +93,9 @@ will be NUL terminated. This can lead to various linear read overflows and other misbehavior due to the missing termination. It also NUL-pads the destination buffer if the source contents are shorter than the destination buffer size, which may be a needless performance penalty for callers using -only NUL-terminated strings. The safe replacement is :c:func:`strscpy`. -(Users of :c:func:`strscpy` still needing NUL-padding will need an -explicit :c:func:`memset` added.) +only NUL-terminated strings. In this case, the safe replacement is +`strscpy()`. If, however, the destination buffer still needs NUL-padding, +the safe replacement is `strscpy_pad()`. If a caller is using non-NUL-terminated strings, :c:func:`strncpy()` can still be used, but destinations should be marked with the `__nonstring diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index bb28b178d929..e6fbf4cf4be4 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -605,6 +605,20 @@ foreach my $entry (keys %deprecated_apis) { } $deprecated_apis_search = "(?:${deprecated_apis_search})"; +our %deprecated_string_apis = ( + "strcpy" => "strscpy", + "strlcpy" => "strscpy", + "strncpy" => "strscpy, strscpy_pad or for non-NUL-terminated strings, strncpy() can still be used, but destinations should be marked with the __nonstring", +); + +#Create a search pattern for all these strings apis to speed up a loop below +our $deprecated_string_apis_search = ""; +foreach my $entry (keys %deprecated_string_apis) { + $deprecated_string_apis_search .= '|' if ($deprecated_string_apis_search ne ""); + $deprecated_string_apis_search .= $entry; +} +$deprecated_string_apis_search = "(?:${deprecated_string_apis_search})"; + our $mode_perms_world_writable = qr{ S_IWUGO | S_IWOTH | @@ -6446,6 +6460,16 @@ sub process { "Deprecated use of '$deprecated_api', prefer '$new_api' instead\n" . $herecurr); } +# check for string deprecated apis + if ($line =~ /\b($deprecated_string_apis_search)\b\s*\(/) { + my $deprecated_string_api = $1; + my $new_api = $deprecated_string_apis{$deprecated_string_api}; + my $msg_level = \&WARN; + $msg_level = \&CHK if ($file); + &{$msg_level}("DEPRECATED_API", + "Deprecated use of '$deprecated_string_api', prefer '$new_api' instead\n" . $herecurr); + } + # check for various structs that are normally const (ops, kgdb, device_tree) # and avoid what seem like struct definitions 'struct foo {' if ($line !~ /\bconst\b/ &&