| Message ID | 20200603233203.1695403-9-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Remove uninitialized_var() macro | expand |
On Wed, 2020-06-03 at 16:32 -0700, Kees Cook wrote: > Using uninitialized_var() is dangerous as it papers over real bugs[1] > (or can in the future), and suppresses unrelated compiler warnings > (e.g. "unused variable"). If the compiler thinks it is uninitialized, > either simply initialize the variable or make compiler changes. > > In preparation for removing[2] the[3] macro[4], effectively revert > commit 16b7f3c89907 ("checkpatch: avoid warning about uninitialized_var()") > and remove all remaining mentions of uninitialized_var(). > > [1] https://lore.kernel.org/lkml/20200603174714.192027-1-glider@google.com/ > [2] https://lore.kernel.org/lkml/CA+55aFw+Vbj0i=1TGqCR5vQkCzWJ0QxK6CernOU6eedsudAixw@mail.gmail.com/ > [3] https://lore.kernel.org/lkml/CA+55aFwgbgqhbp1fkxvRKEpzyR5J8n1vKT1VZdz9knmPuXhOeg@mail.gmail.com/ > [4] https://lore.kernel.org/lkml/CA+55aFz2500WfbKXAx8s67wrm9=yVJu65TpLgN_ybYNv0VEOKA@mail.gmail.com/ nack. see below. I'd prefer a simple revert, but it shouldn't be done here. > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl [] > @@ -4075,7 +4074,7 @@ sub process { > } > > # check for function declarations without arguments like "int foo()" > - if ($line =~ /(\b$Type\s*$Ident)\s*\(\s*\)/) { > + if ($line =~ /(\b$Type\s+$Ident)\s*\(\s*\)/) { This isn't right because $Type includes a possible trailing * where there isn't a space between $Type and $Ident e.g.: int *bar(void); Other than that, fine by me...
On Wed, Jun 03, 2020 at 05:02:29PM -0700, Joe Perches wrote: > On Wed, 2020-06-03 at 16:32 -0700, Kees Cook wrote: > > Using uninitialized_var() is dangerous as it papers over real bugs[1] > > (or can in the future), and suppresses unrelated compiler warnings > > (e.g. "unused variable"). If the compiler thinks it is uninitialized, > > either simply initialize the variable or make compiler changes. > > > > In preparation for removing[2] the[3] macro[4], effectively revert > > commit 16b7f3c89907 ("checkpatch: avoid warning about uninitialized_var()") > > and remove all remaining mentions of uninitialized_var(). > > > > [1] https://lore.kernel.org/lkml/20200603174714.192027-1-glider@google.com/ > > [2] https://lore.kernel.org/lkml/CA+55aFw+Vbj0i=1TGqCR5vQkCzWJ0QxK6CernOU6eedsudAixw@mail.gmail.com/ > > [3] https://lore.kernel.org/lkml/CA+55aFwgbgqhbp1fkxvRKEpzyR5J8n1vKT1VZdz9knmPuXhOeg@mail.gmail.com/ > > [4] https://lore.kernel.org/lkml/CA+55aFz2500WfbKXAx8s67wrm9=yVJu65TpLgN_ybYNv0VEOKA@mail.gmail.com/ > > nack. see below. > > I'd prefer a simple revert, but it shouldn't > be done here. What do you mean? (I can't understand this and "fine by me" below?) > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > [] > > @@ -4075,7 +4074,7 @@ sub process { > > } > > > > # check for function declarations without arguments like "int foo()" > > - if ($line =~ /(\b$Type\s*$Ident)\s*\(\s*\)/) { > > + if ($line =~ /(\b$Type\s+$Ident)\s*\(\s*\)/) { > > This isn't right because $Type includes a possible trailing * > where there isn't a space between $Type and $Ident Ah, hm, that was changed in the mentioned commit: - if ($line =~ /(\b$Type\s+$Ident)\s*\(\s*\)/) { + if ($line =~ /(\b$Type\s*$Ident)\s*\(\s*\)/) { > > e.g.: int *bar(void); > > Other than that, fine by me... Thanks for looking it over! I'll adjust it however you'd like. :)
On Wed, 2020-06-03 at 18:40 -0700, Kees Cook wrote: > On Wed, Jun 03, 2020 at 05:02:29PM -0700, Joe Perches wrote: > > On Wed, 2020-06-03 at 16:32 -0700, Kees Cook wrote: > > > Using uninitialized_var() is dangerous as it papers over real bugs[1] > > > (or can in the future), and suppresses unrelated compiler warnings > > > (e.g. "unused variable"). If the compiler thinks it is uninitialized, > > > either simply initialize the variable or make compiler changes. > > > > > > In preparation for removing[2] the[3] macro[4], effectively revert > > > commit 16b7f3c89907 ("checkpatch: avoid warning about uninitialized_var()") > > > and remove all remaining mentions of uninitialized_var(). > > > > > > [1] https://lore.kernel.org/lkml/20200603174714.192027-1-glider@google.com/ > > > [2] https://lore.kernel.org/lkml/CA+55aFw+Vbj0i=1TGqCR5vQkCzWJ0QxK6CernOU6eedsudAixw@mail.gmail.com/ > > > [3] https://lore.kernel.org/lkml/CA+55aFwgbgqhbp1fkxvRKEpzyR5J8n1vKT1VZdz9knmPuXhOeg@mail.gmail.com/ > > > [4] https://lore.kernel.org/lkml/CA+55aFz2500WfbKXAx8s67wrm9=yVJu65TpLgN_ybYNv0VEOKA@mail.gmail.com/ > > > > nack. see below. > > > > I'd prefer a simple revert, but it shouldn't > > be done here. > > What do you mean? (I can't understand this and "fine by me" below?) I did write "other than that"... I mean that the original commit fixed 2 issues, one with the uninitialized_var addition, and another with the missing void function declaration. I think I found the missing void function bit because the uninitialized_var use looked like a function so I fixed both things at the same time. If you change it, please just remove the bit that checks for uninitialized_var. Thanks, Joe > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > > [] > > > @@ -4075,7 +4074,7 @@ sub process { > > > } > > > > > > # check for function declarations without arguments like "int foo()" > > > - if ($line =~ /(\b$Type\s*$Ident)\s*\(\s*\)/) { > > > + if ($line =~ /(\b$Type\s+$Ident)\s*\(\s*\)/) { > > > > This isn't right because $Type includes a possible trailing * > > where there isn't a space between $Type and $Ident > > Ah, hm, that was changed in the mentioned commit: > > - if ($line =~ /(\b$Type\s+$Ident)\s*\(\s*\)/) { > + if ($line =~ /(\b$Type\s*$Ident)\s*\(\s*\)/) { > > > e.g.: int *bar(void); > > > > Other than that, fine by me... > > Thanks for looking it over! I'll adjust it however you'd like. :) >
On Wed, Jun 03, 2020 at 06:47:13PM -0700, Joe Perches wrote: > On Wed, 2020-06-03 at 18:40 -0700, Kees Cook wrote: > > On Wed, Jun 03, 2020 at 05:02:29PM -0700, Joe Perches wrote: > > > On Wed, 2020-06-03 at 16:32 -0700, Kees Cook wrote: > > > > Using uninitialized_var() is dangerous as it papers over real bugs[1] > > > > (or can in the future), and suppresses unrelated compiler warnings > > > > (e.g. "unused variable"). If the compiler thinks it is uninitialized, > > > > either simply initialize the variable or make compiler changes. > > > > > > > > In preparation for removing[2] the[3] macro[4], effectively revert > > > > commit 16b7f3c89907 ("checkpatch: avoid warning about uninitialized_var()") > > > > and remove all remaining mentions of uninitialized_var(). > > > > > > > > [1] https://lore.kernel.org/lkml/20200603174714.192027-1-glider@google.com/ > > > > [2] https://lore.kernel.org/lkml/CA+55aFw+Vbj0i=1TGqCR5vQkCzWJ0QxK6CernOU6eedsudAixw@mail.gmail.com/ > > > > [3] https://lore.kernel.org/lkml/CA+55aFwgbgqhbp1fkxvRKEpzyR5J8n1vKT1VZdz9knmPuXhOeg@mail.gmail.com/ > > > > [4] https://lore.kernel.org/lkml/CA+55aFz2500WfbKXAx8s67wrm9=yVJu65TpLgN_ybYNv0VEOKA@mail.gmail.com/ > > > > > > nack. see below. > > > > > > I'd prefer a simple revert, but it shouldn't > > > be done here. > > > > What do you mean? (I can't understand this and "fine by me" below?) > > I did write "other than that"... > > I mean that the original commit fixed 2 issues, > one with the uninitialized_var addition, and > another with the missing void function declaration. > > I think I found the missing void function bit because > the uninitialized_var use looked like a function so I > fixed both things at the same time. > > If you change it, please just remove the bit that > checks for uninitialized_var. Ah! Gotcha. Thanks; I will update it. -Kees > > Thanks, Joe > > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > > > [] > > > > @@ -4075,7 +4074,7 @@ sub process { > > > > } > > > > > > > > # check for function declarations without arguments like "int foo()" > > > > - if ($line =~ /(\b$Type\s*$Ident)\s*\(\s*\)/) { > > > > + if ($line =~ /(\b$Type\s+$Ident)\s*\(\s*\)/) { > > > > > > This isn't right because $Type includes a possible trailing * > > > where there isn't a space between $Type and $Ident > > > > Ah, hm, that was changed in the mentioned commit: > > > > - if ($line =~ /(\b$Type\s+$Ident)\s*\(\s*\)/) { > > + if ($line =~ /(\b$Type\s*$Ident)\s*\(\s*\)/) { > > > > > e.g.: int *bar(void); > > > > > > Other than that, fine by me... > > > > Thanks for looking it over! I'll adjust it however you'd like. :) > > >
Hi Kees, can you push that change also to kees/linux.git#kspp/uninit/v5.7/macro ? Thanks in advance. Regards, - Sedat - [1] https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/uninit/v5.7/macro On Thu, Jun 4, 2020 at 4:44 AM Kees Cook <keescook@chromium.org> wrote: > > On Wed, Jun 03, 2020 at 06:47:13PM -0700, Joe Perches wrote: > > On Wed, 2020-06-03 at 18:40 -0700, Kees Cook wrote: > > > On Wed, Jun 03, 2020 at 05:02:29PM -0700, Joe Perches wrote: > > > > On Wed, 2020-06-03 at 16:32 -0700, Kees Cook wrote: > > > > > Using uninitialized_var() is dangerous as it papers over real bugs[1] > > > > > (or can in the future), and suppresses unrelated compiler warnings > > > > > (e.g. "unused variable"). If the compiler thinks it is uninitialized, > > > > > either simply initialize the variable or make compiler changes. > > > > > > > > > > In preparation for removing[2] the[3] macro[4], effectively revert > > > > > commit 16b7f3c89907 ("checkpatch: avoid warning about uninitialized_var()") > > > > > and remove all remaining mentions of uninitialized_var(). > > > > > > > > > > [1] https://lore.kernel.org/lkml/20200603174714.192027-1-glider@google.com/ > > > > > [2] https://lore.kernel.org/lkml/CA+55aFw+Vbj0i=1TGqCR5vQkCzWJ0QxK6CernOU6eedsudAixw@mail.gmail.com/ > > > > > [3] https://lore.kernel.org/lkml/CA+55aFwgbgqhbp1fkxvRKEpzyR5J8n1vKT1VZdz9knmPuXhOeg@mail.gmail.com/ > > > > > [4] https://lore.kernel.org/lkml/CA+55aFz2500WfbKXAx8s67wrm9=yVJu65TpLgN_ybYNv0VEOKA@mail.gmail.com/ > > > > > > > > nack. see below. > > > > > > > > I'd prefer a simple revert, but it shouldn't > > > > be done here. > > > > > > What do you mean? (I can't understand this and "fine by me" below?) > > > > I did write "other than that"... > > > > I mean that the original commit fixed 2 issues, > > one with the uninitialized_var addition, and > > another with the missing void function declaration. > > > > I think I found the missing void function bit because > > the uninitialized_var use looked like a function so I > > fixed both things at the same time. > > > > If you change it, please just remove the bit that > > checks for uninitialized_var. > > Ah! Gotcha. Thanks; I will update it. > > -Kees > > > > > Thanks, Joe > > > > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > > > > [] > > > > > @@ -4075,7 +4074,7 @@ sub process { > > > > > } > > > > > > > > > > # check for function declarations without arguments like "int foo()" > > > > > - if ($line =~ /(\b$Type\s*$Ident)\s*\(\s*\)/) { > > > > > + if ($line =~ /(\b$Type\s+$Ident)\s*\(\s*\)/) { > > > > > > > > This isn't right because $Type includes a possible trailing * > > > > where there isn't a space between $Type and $Ident > > > > > > Ah, hm, that was changed in the mentioned commit: > > > > > > - if ($line =~ /(\b$Type\s+$Ident)\s*\(\s*\)/) { > > > + if ($line =~ /(\b$Type\s*$Ident)\s*\(\s*\)/) { > > > > > > > e.g.: int *bar(void); > > > > > > > > Other than that, fine by me... > > > > > > Thanks for looking it over! I'll adjust it however you'd like. :) > > > > > > > -- > Kees Cook > > -- > You received this message because you are subscribed to the Google Groups "Clang Built Linux" group. > To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/202006031944.9551FAA68E%40keescook.
On Thu, Jun 04, 2020 at 04:53:34AM +0200, Sedat Dilek wrote:
> can you push that change also to kees/linux.git#kspp/uninit/v5.7/macro ?
Done! :)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index b83be177edf0..e9f8146600d0 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -838,7 +838,6 @@ our $FuncArg = qr{$Typecast{0,1}($LvalOrFunc|$Constant|$String)}; our $declaration_macros = qr{(?x: (?:$Storage\s+)?(?:[A-Z_][A-Z0-9]*_){0,2}(?:DEFINE|DECLARE)(?:_[A-Z0-9]+){1,6}\s*\(| (?:$Storage\s+)?[HLP]?LIST_HEAD\s*\(| - (?:$Storage\s+)?${Type}\s+uninitialized_var\s*\(| (?:SKCIPHER_REQUEST|SHASH_DESC|AHASH_REQUEST)_ON_STACK\s*\( )}; @@ -4075,7 +4074,7 @@ sub process { } # check for function declarations without arguments like "int foo()" - if ($line =~ /(\b$Type\s*$Ident)\s*\(\s*\)/) { + if ($line =~ /(\b$Type\s+$Ident)\s*\(\s*\)/) { if (ERROR("FUNCTION_WITHOUT_ARGS", "Bad function definition - $1() should probably be $1(void)\n" . $herecurr) && $fix) { @@ -6271,8 +6270,7 @@ sub process { if (defined $cond) { substr($s, 0, length($cond), ''); } - if ($s =~ /^\s*;/ && - $function_name ne 'uninitialized_var') + if ($s =~ /^\s*;/) { WARN("AVOID_EXTERNS", "externs should be avoided in .c files\n" . $herecurr); @@ -6291,17 +6289,13 @@ sub process { } # check for function declarations that have arguments without identifier names -# while avoiding uninitialized_var(x) if (defined $stat && - $stat =~ /^.\s*(?:extern\s+)?$Type\s*(?:($Ident)|\(\s*\*\s*$Ident\s*\))\s*\(\s*([^{]+)\s*\)\s*;/s && - (!defined($1) || - (defined($1) && $1 ne "uninitialized_var")) && - $2 ne "void") { - my $args = trim($2); + $stat =~ /^.\s*(?:extern\s+)?$Type\s*(?:$Ident|\(\s*\*\s*$Ident\s*\))\s*\(\s*([^{]+)\s*\)\s*;/s && + $1 ne "void") { + my $args = trim($1); while ($args =~ m/\s*($Type\s*(?:$Ident|\(\s*\*\s*$Ident?\s*\)\s*$balanced_parens)?)/g) { my $arg = trim($1); - if ($arg =~ /^$Type$/ && - $arg !~ /enum\s+$Ident$/) { + if ($arg =~ /^$Type$/ && $arg !~ /enum\s+$Ident$/) { WARN("FUNCTION_ARGUMENTS", "function definition argument '$arg' should also have an identifier name\n" . $herecurr); }
Using uninitialized_var() is dangerous as it papers over real bugs[1] (or can in the future), and suppresses unrelated compiler warnings (e.g. "unused variable"). If the compiler thinks it is uninitialized, either simply initialize the variable or make compiler changes. In preparation for removing[2] the[3] macro[4], effectively revert commit 16b7f3c89907 ("checkpatch: avoid warning about uninitialized_var()") and remove all remaining mentions of uninitialized_var(). [1] https://lore.kernel.org/lkml/20200603174714.192027-1-glider@google.com/ [2] https://lore.kernel.org/lkml/CA+55aFw+Vbj0i=1TGqCR5vQkCzWJ0QxK6CernOU6eedsudAixw@mail.gmail.com/ [3] https://lore.kernel.org/lkml/CA+55aFwgbgqhbp1fkxvRKEpzyR5J8n1vKT1VZdz9knmPuXhOeg@mail.gmail.com/ [4] https://lore.kernel.org/lkml/CA+55aFz2500WfbKXAx8s67wrm9=yVJu65TpLgN_ybYNv0VEOKA@mail.gmail.com/ Signed-off-by: Kees Cook <keescook@chromium.org> --- scripts/checkpatch.pl | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-)