[Bug libstdc++/106248] New: operator>>std::basic_istream at boundary condition behave differently in different opt levels

[Bug libstdc++/106248] New: operator>>std::basic_istream at boundary condition behave differently in different opt levels

[Ting.Wang.SH at ibm dot com]

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

            Bug ID: 106248
           Summary: operator>>std::basic_istream at boundary condition
                    behave differently in different opt levels
           Product: gcc
           Version: 11.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: Ting.Wang.SH at ibm dot com
  Target Milestone: ---

SYMPTOM:
Saw operator>>std::basic_istream set ios_base::eofbit differently when buffer
size equals to stream content size under different optimization levels. This is
observed on gcc version 11.2.0 with libstdc++.so.6.0.30, and not observed on
gcc version 9.4.0 with libstdc++.so.6.0.28.

This might not be a bug, however the behavior change under different
optimization levels is a little bit annoying.

An example C++ program is

$ cat a.cc
#include <istream>
#include <sstream>
#include <iostream>

char a_[10];

int main(int argc, char *argv[])
{
  std::basic_string<char, std::char_traits<char>, std::allocator<char> >
input((const char *)"  abcdefghi");
  std::basic_stringbuf<char, std::char_traits<char>, std::allocator<char> >
sbuf(input);
  std::basic_istream<char, std::char_traits<char> > istr(&sbuf);
  istr >> a_;
  std::cout << "istr.rdstate: " << istr.rdstate() << std::endl;
  return 0;
}

$ g++ -O0 -o a.O0 a.cc
$ ./a.O0
istr.rdstate: 2
$ g++ -O3 -o a.O3 a.cc
$ ./a.O3
istr.rdstate: 0

[Bug libstdc++/106248] operator>>std::basic_istream at boundary condition behave differently in different opt levels

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
This is interesting because I get the same behavior in clang with LLVM's
libc++.

Also with -std=c++20 I get the value 0 for all optimization levels.

Maybe there is undefined code here ...

[Bug libstdc++/106248] operator>>std::basic_istream at boundary condition behave differently in different opt levels

[redi at gcc dot gnu.org]

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

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I haven't analyzed it yet but this is probably due to r11-2581-g17abcc77341584

[Bug libstdc++/106248] [11/12/13 Regression] operator>>std::basic_istream at boundary condition behave differently in different opt levels

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2022-07-11
             Status|UNCONFIRMED                 |NEW
            Summary|operator>>std::basic_istrea |[11/12/13 Regression]
                   |m at boundary condition     |operator>>std::basic_istrea
                   |behave differently in       |m at boundary condition
                   |different opt levels        |behave differently in
                   |                            |different opt levels
      Known to fail|                            |11.1.0
     Ever confirmed|0                           |1
      Known to work|                            |10.4.0

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
In C++20 mode the operator>> overload has changed to one that binds to char
(&)[10] and so knows the size of the output buffer. The loop stops reading when
it has written as many chars as will fit in the buffer, and does not set
eofbit:

              if (__extracted < __num - 1
                  && __traits_type::eq_int_type(__c, __eof))
                __err |= ios_base::eofbit;

In pre-C++20 modes, operator>> just binds to a const char* and will overflow it
if the buffer is not big enough. However, libstdc++ now uses
__builtin_object_size to detect the size of the buffer, and so will stop
writing when the buffer is full. That detection using __builtin_object_size
only works when optimization is enabled, which is why we stop before setting
the eofbit when optimizing.

I don't want to just remove the size detection, because it prevents undefined
behaviour. But we need a way to prevent overflow without altering the
observable behaviour for C++17 and earlier.

[Bug libstdc++/106248] [11/12/13 Regression] operator>>std::basic_istream at boundary condition behave differently in different opt levels

[redi at gcc dot gnu.org]

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

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
To put it another way, in C++17 and earlier, writing the buffer stops because
we reach EOF when reading from the istream. If it *didn't* stop there, it would
overflow the buffer and have undefined behaviour. But luckily the program used
the correct buffer size, and EOF happens at just the right time to prevent
overflow. This is the behaviour required by the C++17 standard.

In C++20, writing to the buffer stops because we know the buffer is full and
will not allow it to overflow. That means we don't detect that the input stream
reached EOF, because we don't even try to read any more characters. This is the
behaviour required by the C++20 standard.

The "bug" is that for C++17 mode we use GCC's ability to detect buffer sizes to
provide the safer C++20 behaviour, but that's only possible when optimizing.

[Bug libstdc++/106248] [11/12/13 Regression] operator>>std::basic_istream at boundary condition behave differently in different opt levels

[redi at gcc dot gnu.org]

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

--- Comment #5 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #1)
> This is interesting because I get the same behavior in clang with LLVM's
> libc++.

Are you sure? I do not see any dependency on optimization level when using
libc++.

> Also with -std=c++20 I get the value 0 for all optimization levels.

I *do* see a change in behaviour for C++17 vs C++20 when using libc++, which is
expected for the reasons already given.

[Bug libstdc++/106248] [11/12/13 Regression] operator>>std::basic_istream at boundary condition behave differently in different opt levels

[redi at gcc dot gnu.org]

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

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I think this would restore the previous behaviour without losing the overflow
prevention:

--- a/libstdc++-v3/include/std/istream
+++ b/libstdc++-v3/include/std/istream
@@ -813,8 +813,17 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
       else
        {
          if (__n == (size_t)-1)
-           __n = __gnu_cxx::__numeric_traits<streamsize>::__max;
-         std::__istream_extract(__in, __s, __n / sizeof(_CharT));
+           {
+             __n = __gnu_cxx::__numeric_traits<streamsize>::__max;
+             std::__istream_extract(__in, __s, __n / sizeof(_CharT));
+           }
+         else
+           {
+             std::__istream_extract(__in, __s, __n / sizeof(_CharT));
+             if (__in.good() && _Traits::eq_int_type(__in.rdbuf()->snextc(),
+                                                     _Traits::eof()))
+               __in.setstate(ios_base::eofbit);
+           }
        }
       return __in;
     }

[Bug libstdc++/106248] [11/12/13 Regression] operator>>std::basic_istream at boundary condition behave differently in different opt levels

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |redi at gcc dot gnu.org
             Status|NEW                         |ASSIGNED
   Target Milestone|---                         |11.4

[Bug libstdc++/106248] [11/12/13 Regression] operator>>std::basic_istream at boundary condition behave differently in different opt levels

[redi at gcc dot gnu.org]

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

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Comment 6 isn't right (it should be sgetc not snextc, and it needs to consider
the stream's field width) but I'm testing a working patch now.

[Bug libstdc++/106248] [11/12/13 Regression] operator>>std::basic_istream at boundary condition behave differently in different opt levels

[cvs-commit at gcc dot gnu.org]

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

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:5ae74944af1de032d4a27fad4a2287bd3a2163fd

commit r13-1651-g5ae74944af1de032d4a27fad4a2287bd3a2163fd
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Jul 12 11:18:47 2022 +0100

    libstdc++: Check for EOF if extraction avoids buffer overflow [PR106248]

    In r11-2581-g17abcc77341584 (for LWG 2499) I added overflow checks to
    the pre-C++20 operator>>(istream&, char*) overload.  Those checks can
    cause extraction to stop after filling the buffer, where previously it
    would have tried to extract another character and stopped at EOF. When
    that happens we no longer set eofbit in the stream state, which is
    consistent with the behaviour of the new C++20 overload, but is an
    observable and unexpected change in the C++17 behaviour. What makes it
    worse is that the behaviour change is dependent on optimization, because
    __builtin_object_size is used to detect the buffer size and that only
    works when optimizing.

    To avoid the unexpected and optimization-dependent change in behaviour,
    set eofbit manually if we stopped extracting because of the buffer size
    check, but had reached EOF anyway. If the stream's rdstate() != goodbit
    or width() is non-zero and smaller than the buffer, there's nothing to
    do. Otherwise, we filled the buffer and need to check for EOF, and maybe
    set eofbit.

    The new check is guarded by #ifdef __OPTIMIZE__ because otherwise
    __builtin_object_size is useless. There's no point compiling and
    emitting dead code that can't be eliminated because we're not
    optimizing.

    We could add extra checks that the next character in the buffer is not
    whitespace, to detect the case where we stopped early and prevented a
    buffer overflow that would have happened otherwise. That would allow us
    to assert or set badbit in the stream state when undefined behaviour was
    prevented. However, those extra checks would increase the size of the
    function, potentially reducing the likelihood of it being inlined, and
    so making the buffer size detection less reliable. It seems preferable
    to prevent UB and silently truncate, rather than miss the UB and allow
    the overflow to happen.

    libstdc++-v3/ChangeLog:

            PR libstdc++/106248
            * include/std/istream [C++17] (operator>>(istream&, char*)):
            Set eofbit if we stopped extracting at EOF.
            *
testsuite/27_io/basic_istream/extractors_character/char/pr106248.cc:
            New test.
            *
testsuite/27_io/basic_istream/extractors_character/wchar_t/pr106248.cc:
            New test.

[Bug libstdc++/106248] [11/12 Regression] operator>>std::basic_istream at boundary condition behave differently in different opt levels

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to fail|                            |12.1.0
            Summary|[11/12/13 Regression]       |[11/12 Regression]
                   |operator>>std::basic_istrea |operator>>std::basic_istrea
                   |m at boundary condition     |m at boundary condition
                   |behave differently in       |behave differently in
                   |different opt levels        |different opt levels

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Fixed on trunk only so far.

[Bug libstdc++/106248] [11/12 Regression] operator>>std::basic_istream at boundary condition behave differently in different opt levels

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2

[Bug libstdc++/106248] [11/12 Regression] operator>>std::basic_istream at boundary condition behave differently in different opt levels

[cvs-commit at gcc dot gnu.org]

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

--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Jonathan Wakely
<redi@gcc.gnu.org>:

https://gcc.gnu.org/g:7a0ed28d4feb450f1ede5b52b57793a5df5b19fe

commit r12-8659-g7a0ed28d4feb450f1ede5b52b57793a5df5b19fe
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Jul 12 11:18:47 2022 +0100

    libstdc++: Check for EOF if extraction avoids buffer overflow [PR106248]

    In r11-2581-g17abcc77341584 (for LWG 2499) I added overflow checks to
    the pre-C++20 operator>>(istream&, char*) overload.  Those checks can
    cause extraction to stop after filling the buffer, where previously it
    would have tried to extract another character and stopped at EOF. When
    that happens we no longer set eofbit in the stream state, which is
    consistent with the behaviour of the new C++20 overload, but is an
    observable and unexpected change in the C++17 behaviour. What makes it
    worse is that the behaviour change is dependent on optimization, because
    __builtin_object_size is used to detect the buffer size and that only
    works when optimizing.

    To avoid the unexpected and optimization-dependent change in behaviour,
    set eofbit manually if we stopped extracting because of the buffer size
    check, but had reached EOF anyway. If the stream's rdstate() != goodbit
    or width() is non-zero and smaller than the buffer, there's nothing to
    do. Otherwise, we filled the buffer and need to check for EOF, and maybe
    set eofbit.

    The new check is guarded by #ifdef __OPTIMIZE__ because otherwise
    __builtin_object_size is useless. There's no point compiling and
    emitting dead code that can't be eliminated because we're not
    optimizing.

    We could add extra checks that the next character in the buffer is not
    whitespace, to detect the case where we stopped early and prevented a
    buffer overflow that would have happened otherwise. That would allow us
    to assert or set badbit in the stream state when undefined behaviour was
    prevented. However, those extra checks would increase the size of the
    function, potentially reducing the likelihood of it being inlined, and
    so making the buffer size detection less reliable. It seems preferable
    to prevent UB and silently truncate, rather than miss the UB and allow
    the overflow to happen.

    libstdc++-v3/ChangeLog:

            PR libstdc++/106248
            * include/std/istream [C++17] (operator>>(istream&, char*)):
            Set eofbit if we stopped extracting at EOF.
            *
testsuite/27_io/basic_istream/extractors_character/char/pr106248.cc:
            New test.
            *
testsuite/27_io/basic_istream/extractors_character/wchar_t/pr106248.cc:
            New test.

    (cherry picked from commit 5ae74944af1de032d4a27fad4a2287bd3a2163fd)

[Bug libstdc++/106248] [11 Regression] operator>>std::basic_istream at boundary condition behave differently in different opt levels

[redi at gcc dot gnu.org]

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

--- Comment #11 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Backported for 12.2

[Bug libstdc++/106248] [11 Regression] operator>>std::basic_istream at boundary condition behave differently in different opt levels

[cvs-commit at gcc dot gnu.org]

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

--- Comment #12 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Jonathan Wakely
<redi@gcc.gnu.org>:

https://gcc.gnu.org/g:3d17eaabb3e771472d7bfa1f654564a5db57f5ef

commit r11-10241-g3d17eaabb3e771472d7bfa1f654564a5db57f5ef
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Jul 12 11:18:47 2022 +0100

    libstdc++: Check for EOF if extraction avoids buffer overflow [PR106248]

    In r11-2581-g17abcc77341584 (for LWG 2499) I added overflow checks to
    the pre-C++20 operator>>(istream&, char*) overload.  Those checks can
    cause extraction to stop after filling the buffer, where previously it
    would have tried to extract another character and stopped at EOF. When
    that happens we no longer set eofbit in the stream state, which is
    consistent with the behaviour of the new C++20 overload, but is an
    observable and unexpected change in the C++17 behaviour. What makes it
    worse is that the behaviour change is dependent on optimization, because
    __builtin_object_size is used to detect the buffer size and that only
    works when optimizing.

    To avoid the unexpected and optimization-dependent change in behaviour,
    set eofbit manually if we stopped extracting because of the buffer size
    check, but had reached EOF anyway. If the stream's rdstate() != goodbit
    or width() is non-zero and smaller than the buffer, there's nothing to
    do. Otherwise, we filled the buffer and need to check for EOF, and maybe
    set eofbit.

    The new check is guarded by #ifdef __OPTIMIZE__ because otherwise
    __builtin_object_size is useless. There's no point compiling and
    emitting dead code that can't be eliminated because we're not
    optimizing.

    We could add extra checks that the next character in the buffer is not
    whitespace, to detect the case where we stopped early and prevented a
    buffer overflow that would have happened otherwise. That would allow us
    to assert or set badbit in the stream state when undefined behaviour was
    prevented. However, those extra checks would increase the size of the
    function, potentially reducing the likelihood of it being inlined, and
    so making the buffer size detection less reliable. It seems preferable
    to prevent UB and silently truncate, rather than miss the UB and allow
    the overflow to happen.

    libstdc++-v3/ChangeLog:

            PR libstdc++/106248
            * include/std/istream [C++17] (operator>>(istream&, char*)):
            Set eofbit if we stopped extracting at EOF.
            *
testsuite/27_io/basic_istream/extractors_character/char/pr106248.cc:
            New test.
            *
testsuite/27_io/basic_istream/extractors_character/wchar_t/pr106248.cc:
            New test.

    (cherry picked from commit 5ae74944af1de032d4a27fad4a2287bd3a2163fd)

[Bug libstdc++/106248] [11 Regression] operator>>std::basic_istream at boundary condition behave differently in different opt levels

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #13 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Fixed for 11.4