[Bug c++/103939] New: memset with sizeof in wrong place not detected ?

[Bug c++/103939] New: memset with sizeof in wrong place not detected ?

[dcb314 at hotmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103939

            Bug ID: 103939
           Summary: memset with sizeof in wrong place not detected ?
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: dcb314 at hotmail dot com
  Target Milestone: ---

For this C++ code:

$ more jan7d.cc

#include <cstring>

struct S
{
        int a;
        int b;
        int c;
        int d;
};

void f( S * ps)
{
   memset(ps, sizeof(S), 0xff);
}

compiled with recent gcc trunk:

$ /home/dcb/gcc/results/bin/gcc -c -g -O2 -Wall -Wextra -pedantic jan7d.cc
$ 

compiled by clang-13:

$ /usr/bin/clang++ -c -g -O2 -Wall -Wextra -pedantic jan7d.cc
jan7d.cc:14:15: warning: setting buffer to a 'sizeof' expression; did you mean
to transpose the last two arguments? [-Wmemset-transposed-args]
   memset(ps, sizeof(S), 0xff);
              ^
jan7d.cc:14:15: note: cast the second argument to 'int' to silence
1 warning generated.

[Bug c++/103939] memset with sizeof in wrong place not detected ?

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103939

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |enhancement
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2022-01-07
           Keywords|                            |diagnostic
             Blocks|                            |89863
             Status|UNCONFIRMED                 |NEW

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
The docs explain why GCC's -Wmemset-transposed-args doesn't warn:

  Warn for suspicious calls to the "memset" built-in function where the second
  argument is not zero and the third argument is zero.  For example, the call
  "memset (buf, sizeof buf, 0)" is diagnosed because "memset (buf, 0, sizeof
  buf)" was meant instead.  The diagnostic is only emitted if the third
argument
  is a literal zero.

So not warning for your case is by design.

But it probably makes sense to warn if the second argument is a sizeof
expression, whatever the value of the third argument.


Referenced Bugs:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89863
[Bug 89863] [meta-bug] Issues in gcc that other static analyzers (cppcheck,
clang-static-analyzer, PVS-studio) find that gcc misses

[Bug c++/103939] memset with sizeof in wrong place not detected ?

[egallager at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103939

Eric Gallager <egallager at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |egallager at gcc dot gnu.org

--- Comment #2 from Eric Gallager <egallager at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #1)
> The docs explain why GCC's -Wmemset-transposed-args doesn't warn:
> 
>   Warn for suspicious calls to the "memset" built-in function where the
> second
>   argument is not zero and the third argument is zero.  For example, the call
>   "memset (buf, sizeof buf, 0)" is diagnosed because "memset (buf, 0, sizeof
>   buf)" was meant instead.  The diagnostic is only emitted if the third
> argument
>   is a literal zero.
> 
> So not warning for your case is by design.
> 
> But it probably makes sense to warn if the second argument is a sizeof
> expression, whatever the value of the third argument.

What if both the second and third arguments are 'sizeof' expressions?

[Bug c++/103939] memset with sizeof in wrong place not detected ?

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103939

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Has anybody ever written such code?

[Bug c++/103939] memset with sizeof in wrong place not detected ?

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103939

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I doubt they have, but if it's a concern, warn when the second argument is a
sizeof expression and the third is any integer literal. That should catch cases
where it matters.

[Bug c++/103939] memset with sizeof in wrong place not detected ?

[dcb314 at hotmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103939

--- Comment #5 from David Binderman <dcb314 at hotmail dot com> ---
(In reply to Jonathan Wakely from comment #3)
> Has anybody ever written such code?

Yes, it is part of Fedora rawhide right now. 

>From packages with names starting 0-9a-d, package dvdisaster has it.

It happens often enough that clang put a warning in for it.

[Bug c++/103939] memset with sizeof in wrong place not detected ?

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103939

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I was replying to comment 2: "What if both the second and third arguments are
'sizeof' expressions?"

Are you saying Fedora has code doing memset(ps, sizeof(a), sizeof(b)) ?

[Bug c++/103939] memset with sizeof in wrong place not detected ?

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103939

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I see no such usage in dvdisaster, so I assume you were talking about the
original example you gave in comment 0. Nobody is disputing the usefulness of
that warning. I was questioning whether we need to care about
memset(p,sizeof(a),sizeof(b)).

[Bug c++/103939] memset with sizeof in wrong place not detected ?

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103939

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
memset (whatever, sizeof(whatever), 0);
is a common mistake, that is why gcc has a warning for it too.
But your report is the first one to suggest that even other variants
happen in real-world.  The thing is, memset (whatver, sizeof (...), 0)
is clearly a bug, memset with literal 0 as last argument just shouldn't be
added because it doesn't do anything.
memset (whatever, sizeof (...), 0xff); could be valid thing to do if you want
to initialize buf[0xff] to size of something, so there is bigger chance of
false positives.

[Bug c++/103939] memset with sizeof in wrong place not detected ?

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103939

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
The fedora package has:

dvdisaster-0.79.5/read-linear.c:   memset(rc, sizeof(read_closure), 0xff); 

The chance of false positives still seems pretty small to me.

[Bug c++/103939] memset with sizeof in wrong place not detected ?

[schwab@linux-m68k.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103939

--- Comment #10 from Andreas Schwab <schwab@linux-m68k.org> ---
If the second argument matches the size of the dereferenced first argument, the
probability of a false positive should be quite small.

[Bug c++/103939] memset with sizeof in wrong place not detected ?

[dcb314 at hotmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103939

--- Comment #11 from David Binderman <dcb314 at hotmail dot com> ---
(In reply to Jonathan Wakely from comment #9)
> The fedora package has:
> 
> dvdisaster-0.79.5/read-linear.c:   memset(rc, sizeof(read_closure), 0xff); 
> 
> The chance of false positives still seems pretty small to me.

Agreed. Anything that looks like

    memset( p, sizeof( something), small integer representable in a char);

looks like a newbie error to me.