[Bug middle-end/104854] New: [11 Regression] -Wstringop-overread should not warn for strnlen and strndup

[Bug middle-end/104854] New: [11 Regression] -Wstringop-overread should not warn for strnlen and strndup

[siddhesh at gcc dot gnu.org]

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

            Bug ID: 104854
           Summary: [11 Regression] -Wstringop-overread should not warn
                    for strnlen and strndup
           Product: gcc
           Version: 11.2.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: middle-end
          Assignee: unassigned at gcc dot gnu.org
          Reporter: siddhesh at gcc dot gnu.org
  Target Milestone: ---

The only times that strnlen and strndup can result in an actual overread is:

- The source is zero sized

or 

- The source is not NULL terminated

In the current state something as trivial as the following:

char *
foo (size_t size)
{
  return __builtin_strndup ("test", size);
}

char *
bar (void)
{
  return foo (20);
}

issues a warning when the code is harmless.  This is probably better suited as
a static analysis heuristic/suggestion than as a compiler warning that tends to
 suggest that something is more likely wrong than not.

[Bug middle-end/104854] [11/12 Regression] -Wstringop-overread should not warn for strnlen and strndup

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[11 Regression]             |[11/12 Regression]
                   |-Wstringop-overread should  |-Wstringop-overread should
                   |not warn for strnlen and    |not warn for strnlen and
                   |strndup                     |strndup
           Keywords|                            |diagnostic
   Target Milestone|---                         |11.3

[Bug middle-end/104854] [11/12 Regression] -Wstringop-overread should not warn for strnlen and strndup

[dmalcolm at gcc dot gnu.org]

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

David Malcolm <dmalcolm at gcc dot gnu.org> changed:

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

--- Comment #1 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Compiler Explorer link for the above (with -fanalyzer -Wall -Wstringop-overread
-O2; -O2 seems to be needed to trigger it):

  https://godbolt.org/z/jcEdxfEv9

In function 'foo',
    inlined from 'bar' at <source>:12:10:
<source>:6:10: warning: '__builtin_strndup' specified bound 20 exceeds source
size 5 [-Wstringop-overread]
    6 |   return __builtin_strndup ("test", size);
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[Bug middle-end/104854] [11/12 Regression] -Wstringop-overread should not warn for strnlen and strndup

[siddhesh at gcc dot gnu.org]

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

--- Comment #2 from Siddhesh Poyarekar <siddhesh at gcc dot gnu.org> ---
(In reply to David Malcolm from comment #1)
> Compiler Explorer link for the above (with -fanalyzer -Wall
> -Wstringop-overread -O2; -O2 seems to be needed to trigger it):

Ah yes, sorry, I pasted an older reproducer.  This:

char *
foo (void)
{
  return __builtin_strndup ("test", 8);
}

doesn't need -O2.


$ gcc -S -Wall ../test.c
../test.c: In function ‘foo’:
../test.c:5:10: warning: ‘__builtin_strndup’ specified bound 8 exceeds source
size 5 [-Wstringop-overread]
    5 |   return __builtin_strndup ("test", 8);
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[Bug middle-end/104854] -Wstringop-overread should not warn for strnlen and strndup

[msebor at gcc dot gnu.org]

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

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |msebor at gcc dot gnu.org
            Summary|[11/12 Regression]          |-Wstringop-overread should
                   |-Wstringop-overread should  |not warn for strnlen and
                   |not warn for strnlen and    |strndup
                   |strndup                     |

--- Comment #3 from Martin Sebor <msebor at gcc dot gnu.org> ---
The same warning is also issued for some calls to memchr(), strncmp() and
probably other built-in functions as well.  For example:

  const char a[] = "123";

  char* f (int i)
  {
    return __builtin_memchr (a + i, '3', 7);
  }

  warning: ‘__builtin_memchr’ specified bound 7 exceeds source size 4
[-Wstringop-overread]
      5 |   return __builtin_memchr (a + i, '3', 7);
        |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  y.c:1:12: note: source object allocated here
      1 | const char a[] = "123";
        |            ^

In all these cases the warnings are intentional (i.e., it's a feature, and so
not a regression).  Their purpose is to point out what could be a coding
mistake.  With strndup(), since the use case for it rather than strdup() is to
copy an initial substring, specifying a bound that's larger than the length of
the string is pointless.

In general, the GCC manual documents warnings as

  diagnostic messages that report constructions that are not inherently
erroneous but that are risky or suggest there may have been an error. 

So not all instances of any warning should be expected to indicate errors.  In
fact, many are known to be benign by design (for example, all instances of
-Wchar-subscripts are benign when -funsigned-char is used; many instance of
-Waddress are also benign, as are many -Wparentheses, etc.).  And although most
middle end warnings tend to be designed to trigger only for invalid statements
(i.e., statements that have undefined behavior if reached during execution),
some do also trigger for code that's strictly valid but suspect.  Besides the
-Wstringop-overread examples here, other such warnings include dynamic
allocation calls with sizes in excess of PTRDIFF_MAX
(-Walloc-size-larger-than), returning the address of a local variable
(-Wreturn-local-addr), or in GCC 12, storing the address of a local variable in
an extern pointer (-Wdangling-pointer).

Deciding what code is suspect enough to warn and under what option (-Wall or
-Wextra) is a judgment call; different people have very different ideas.

[Bug middle-end/104854] -Wstringop-overread should not warn for strnlen, strndup and strncmp

[siddhesh at gcc dot gnu.org]

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

Siddhesh Poyarekar <siddhesh at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|-Wstringop-overread should  |-Wstringop-overread should
                   |not warn for strnlen and    |not warn for strnlen,
                   |strndup                     |strndup and strncmp
           Assignee|unassigned at gcc dot gnu.org      |siddhesh at gcc dot gnu.org

--- Comment #4 from Siddhesh Poyarekar <siddhesh at gcc dot gnu.org> ---
(In reply to Martin Sebor from comment #3)
> The same warning is also issued for some calls to memchr(), strncmp() and
> probably other built-in functions as well.  For example:
> 
>   const char a[] = "123";
> 
>   char* f (int i)
>   {
>     return __builtin_memchr (a + i, '3', 7);
>   }
> 
>   warning: ‘__builtin_memchr’ specified bound 7 exceeds source size 4
> [-Wstringop-overread]
>       5 |   return __builtin_memchr (a + i, '3', 7);
>         |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   y.c:1:12: note: source object allocated here
>       1 | const char a[] = "123";
>         |            ^

The warning is arguably legitimate (not this one of course, since it's evident
at compile time that the operation is safe but for non-const `a` it may not be)
for memchr because it operates on an object, not string and will traverse all
of the object for a matching char to the extent of the object size.  The
*string* functions are not the same in that context.

> In all these cases the warnings are intentional (i.e., it's a feature, and
> so not a regression).  Their purpose is to point out what could be a coding
> mistake.  With strndup(), since the use case for it rather than strdup() is
> to copy an initial substring, specifying a bound that's larger than the
> length of the string is pointless.
> 
> In general, the GCC manual documents warnings as
> 
>   diagnostic messages that report constructions that are not inherently
> erroneous but that are risky or suggest there may have been an error. 

I don't think these strnlen or strndup warnings point to *risky* or potentially
erroneous code; at best they point to instances where a specific protection is
absent, i.e. the behaviour reduces to strlen and strdup respectively, which is
much more benign than what it currently suggests.  That kind of warning seems
more suited to a static analyzer and not a compiler IMO.

Besides, the core cause of a potential overread here is not because the size
argument is larger but because the string may not be NULL terminated.  It may
make more sense to invest in that aspect of risky behaviour than the length for
these functions.

> So not all instances of any warning should be expected to indicate errors. 
> In fact, many are known to be benign by design (for example, all instances
> of -Wchar-subscripts are benign when -funsigned-char is used; many instance
> of -Waddress are also benign, as are many -Wparentheses, etc.).  And

They're not clubbed in with potentially important warnings though, which is a
key distinction.  For example, one could mark -Wstirngop-overread as important
warnings but not -Wparentheses, but the high noise could make it difficult to
actually do so.

> although most middle end warnings tend to be designed to trigger only for
> invalid statements (i.e., statements that have undefined behavior if reached
> during execution), some do also trigger for code that's strictly valid but
> suspect.  Besides the -Wstringop-overread examples here, other such warnings
> include dynamic allocation calls with sizes in excess of PTRDIFF_MAX
> (-Walloc-size-larger-than), returning the address of a local variable
> (-Wreturn-local-addr), or in GCC 12, storing the address of a local variable
> in an extern pointer (-Wdangling-pointer).
> 
> Deciding what code is suspect enough to warn and under what option (-Wall or
> -Wextra) is a judgment call; different people have very different ideas.

I'm testing a patch to resolve this.  I noticed strncmp late but it seems to
match this category as well, where a too-long length only reduces the max
protection provided by the n-versions of the string functions.

[Bug middle-end/104854] -Wstringop-overread should not warn for strnlen, strndup and strncmp

[msebor at gcc dot gnu.org]

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

--- Comment #5 from Martin Sebor <msebor at gcc dot gnu.org> ---
It would be useful to separate these warnings into multiple levels: level 1 for
invalid code, and higher levels for suspicious (or pointless) code, similarly
to -Wformat-overflow.

[Bug middle-end/104854] -Wstringop-overread should not warn for strnlen, strndup and strncmp

[siddhesh at gcc dot gnu.org]

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

--- Comment #6 from Siddhesh Poyarekar <siddhesh at gcc dot gnu.org> ---
(In reply to Martin Sebor from comment #5)
> It would be useful to separate these warnings into multiple levels: level 1
> for invalid code, and higher levels for suspicious (or pointless) code,
> similarly to -Wformat-overflow.

I think the analyzer is a great level for the higher level heuristics, with ME
warnings only sticking to level 1. Adding levels within ME warnings seems
unnecessary.  ISTM that users tend to *expect* false positives (to some sane
extent) when doing static analysis but are much less tolerant of those during
usual builds.

[Bug middle-end/104854] -Wstringop-overread should not warn for strnlen, strndup and strncmp

[msebor at gcc dot gnu.org]

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

--- Comment #7 from Martin Sebor <msebor at gcc dot gnu.org> ---
Moving warnings into the analyzer and scaling it up to be able to run by
default, during development, sounds like a good long-term plan.  Until that
happens, rather than gratuitously removing warnings that we've added over the
years, just because they fall short of the ideal 100% efficacy (as has been
known and documented), making them easier to control seems like a better
approach.

[Bug middle-end/104854] -Wstringop-overread should not warn for strnlen, strndup and strncmp

[siddhesh at gcc dot gnu.org]

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

--- Comment #8 from Siddhesh Poyarekar <siddhesh at gcc dot gnu.org> ---
(In reply to Martin Sebor from comment #7)
> Moving warnings into the analyzer and scaling it up to be able to run by
> default, during development, sounds like a good long-term plan.  Until that

That's not quite what I'm suggesting here.  I'm not a 100% convinced that those
are the right heuristics at all; the size argument for strnlen, strndup and
strncmp does not intend to describe the size of the passed strings.  It is only
recommended security practice that the *n* variant functions be used instead of
their unconstrained relatives to mitigate overflows.  In fact in more common
cases the size argument (especially in case of strnlen and strncmp) may
describe a completely different buffer or some other application-specific
property.

This is different from the -Wformat-overflow, where there is a clear
relationship between buffer, the format string and the string representation of
input numbers and we're only tweaking is the optimism level of the warnings. 
So it is not just a question of levels of verosity/paranoia.

In that context, using size to describe the underlying buffer of the source
only makes sense only for a subset of uses, making this heuristic quite noisy. 
So what I'm actually saying is: the heuristic is too noisy but if we insist on
keeping it, it makes sense as an analyzer warning where the user *chooses* to
look for pessimistic scenarios and is more tolerant of noisy heuristics.

> happens, rather than gratuitously removing warnings that we've added over
> the years, just because they fall short of the ideal 100% efficacy (as has
> been known and documented), making them easier to control seems like a
> better approach.

It's not just a matter of efficacy here IMO.  The heuristic for strnlen,
strncmp and strndup overreads is too loose for it to be taken seriously.

[Bug middle-end/104854] -Wstringop-overread should not warn for strnlen, strndup and strncmp

[dmalcolm at gcc dot gnu.org]

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

--- Comment #9 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
(In reply to Siddhesh Poyarekar from comment #8)
> (In reply to Martin Sebor from comment #7)
> > Moving warnings into the analyzer and scaling it up to be able to run by
> > default, during development, sounds like a good long-term plan.  Until that
> 
> That's not quite what I'm suggesting here.  I'm not a 100% convinced that
> those are the right heuristics at all; the size argument for strnlen,
> strndup and strncmp does not intend to describe the size of the passed
> strings.  It is only recommended security practice that the *n* variant
> functions be used instead of their unconstrained relatives to mitigate
> overflows.  In fact in more common cases the size argument (especially in
> case of strnlen and strncmp) may describe a completely different buffer or
> some other application-specific property.
> 
> This is different from the -Wformat-overflow, where there is a clear
> relationship between buffer, the format string and the string representation
> of input numbers and we're only tweaking is the optimism level of the
> warnings.  So it is not just a question of levels of verosity/paranoia.
> 
> In that context, using size to describe the underlying buffer of the source
> only makes sense only for a subset of uses, making this heuristic quite
> noisy.  So what I'm actually saying is: the heuristic is too noisy but if we
> insist on keeping it, it makes sense as an analyzer warning where the user
> *chooses* to look for pessimistic scenarios and is more tolerant of noisy
> heuristics.

Right now -fanalyzer enables all of the various -Wanalyzer-* warnings by
default [1], and in theory all of them only emit a diagnostic for the case when
the analyzer "thinks" there's a definite problem.  There may be bugs in the
analyzer, of course.  I'm a bit wary of the above sentence, as it suggests that
the analyzer should be the place to put noisy diagnostics.

Looking at the GCC UX guidelines:
  https://gcc.gnu.org/onlinedocs/gccint/Guidelines-for-Diagnostics.html
note "The user’s attention is important": if a diagnostic is too noisy, the
user will either turn it off, or stop paying attention.

The distinction I've been making for -fanalyzer is that -fanalyzer enables a
more expensive (path-based) analysis of the user's code, and will slow down the
user's compile-time, with various warnings tied to that, i.e. I've been
messaging it primarily as a compile-time tradeoff for extra warnings that
otherwise would be too slow to implement, rather than a signal:noise ratio
tradeoff.  -fanalyzer can generate false positives, but I've been trying to
drive that down via bugfixes (it's also relatively new code)

[1] apart from -Wanalyzer-too-complex, but that's more of an implementation
detail.

> 
> > happens, rather than gratuitously removing warnings that we've added over
> > the years, just because they fall short of the ideal 100% efficacy (as has
> > been known and documented), making them easier to control seems like a
> > better approach.
> 
> It's not just a matter of efficacy here IMO.  The heuristic for strnlen,
> strncmp and strndup overreads is too loose for it to be taken seriously.

[Bug middle-end/104854] -Wstringop-overread should not warn for strnlen, strndup and strncmp

[msebor at gcc dot gnu.org]

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

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |INVALID

--- Comment #10 from Martin Sebor <msebor at gcc dot gnu.org> ---
I forgot this strndup pitfall that POSIX cautions about in its APPLICATION
USAGE and that the warning helps avoid:

  Implementations are free to malloc() a buffer containing either (size + 1)
bytes or (strnlen(s, size) + 1) bytes.  Applications should not assume that
strndup() will allocate (size + 1) bytes when strlen(s) is smaller than size.

Most implementations, including Glibc, only allocate strnlen (s, size) (i.e.,
less than size if s is shorter).

Since the only motivating test case here is strndup and since it turned out
that the patch submitted for this report was based on a misunderstanding of the
warning (https://gcc.gnu.org/pipermail/gcc-patches/2022-March/591926.html) and
didn't do anything for strndup I'm going to resolve this as invalid.

If you want to raise problems about the warning for strnlen or strncmp please
open separate bugs and attach test cases, preferably from real code.  None of
those provided by Steve Grubb appears to have anything to do with strnlen or
strncmp.