[Bug libstdc++/98001] New: ext/stdio_filebuf/char/79820.cc is broken

[Bug libstdc++/98001] New: ext/stdio_filebuf/char/79820.cc is broken

[rguenth at gcc dot gnu.org]

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

            Bug ID: 98001
           Summary: ext/stdio_filebuf/char/79820.cc is broken
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: rguenth at gcc dot gnu.org
  Target Milestone: ---

The testcase reliably segfaults for me when run with

MALLOC_PERTURB_=69
MALLOC_CHECK_=3

in the environment.  It then segfaults doing

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff74ec5bb in fflush () from /lib64/libc.so.6
(gdb) up
#1  0x00007ffff7b06175 in std::__basic_file<char>::sys_open
(this=this@entry=0x7fffffffe678, 
    __file=__file@entry=0x614c20) at basic_file.cc:202
202               __err = fflush(__file);
(gdb) l
197           {
198             int __err, __save_errno = errno;
199             // POSIX guarantees that fflush sets errno on error, but C
doesn't.
200             errno = 0;
201             do
202               __err = fflush(__file);
203             while (__err && errno == EINTR);

the testcase passes a FILE * that has been fclosed() to the
__gnu_cxx::stdio_filebuf<char> CTOR which then invokes fflush on it.

fclose() is documented as

RETURN VALUE
       Upon  successful completion, 0 is returned.  Otherwise, EOF is returned
       and errno is set to indicate the error.  In either  case,  any  further
       access  (including  another  call to fclose()) to the stream results in
       undefined behavior.

so the testcase invokes undefined behavior.  I don't think there's any way
to query whether a FILE * is valid or not for the standard library.

[Bug libstdc++/98001] ext/stdio_filebuf/char/79820.cc is broken

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

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

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Yeah, FILE * handles are or can be like malloced/freed pointers, where fopen
allocates them and fclose frees them, accessing those outside of that range can
lead to dereferencing a freed pointer.

[Bug libstdc++/98001] ext/stdio_filebuf/char/79820.cc is broken

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2020-11-26
             Status|UNCONFIRMED                 |NEW

[Bug libstdc++/98001] ext/stdio_filebuf/char/79820.cc is broken

[redi at gcc dot gnu.org]

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

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

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I don't think we actually need fflush(__file) to set errno in order to verify
that the library doesn't set it to zero.

So this should be fine:

--- a/libstdc++-v3/testsuite/ext/stdio_filebuf/char/79820.cc
+++ b/libstdc++-v3/testsuite/ext/stdio_filebuf/char/79820.cc
@@ -26,10 +26,10 @@ void
 test01()
 {
   FILE* f = std::fopen("79820.txt", "w");
-  std::fclose(f);
   errno = 127;
   __gnu_cxx::stdio_filebuf<char> b(f, std::ios::out, BUFSIZ);
   VERIFY(errno == 127); // PR libstdc++/79820
+  std::fclose(f);
 }

 int

[Bug libstdc++/98001] ext/stdio_filebuf/char/79820.cc is broken

[rguenther at suse dot de]

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

--- Comment #3 from rguenther at suse dot de <rguenther at suse dot de> ---
On Thu, 26 Nov 2020, redi at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98001
> 
> 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
> 
> --- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
> I don't think we actually need fflush(__file) to set errno in order to verify
> that the library doesn't set it to zero.
> 
> So this should be fine:
> 
> --- a/libstdc++-v3/testsuite/ext/stdio_filebuf/char/79820.cc
> +++ b/libstdc++-v3/testsuite/ext/stdio_filebuf/char/79820.cc
> @@ -26,10 +26,10 @@ void
>  test01()
>  {
>    FILE* f = std::fopen("79820.txt", "w");
> -  std::fclose(f);
>    errno = 127;
>    __gnu_cxx::stdio_filebuf<char> b(f, std::ios::out, BUFSIZ);
>    VERIFY(errno == 127); // PR libstdc++/79820
> +  std::fclose(f);
>  }

Is the DTOR of stdio_filebuf fine with the file being closed
under it?  For a fix in our GCC 7 tree I wrapped it in
its own scope to ensure the DTOR is called before fclose

>  int
> 
>

[Bug libstdc++/98001] ext/stdio_filebuf/char/79820.cc is broken

[redi at gcc dot gnu.org]

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

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Yes, when the filebuf is given a FILE* it doesn't own it, and so the destructor
doesn't touch it:

  __basic_file<char>*
  __basic_file<char>::close()
  {
    __basic_file* __ret = static_cast<__basic_file*>(NULL);
    if (this->is_open())
      {
        int __err = 0;
        if (_M_cfile_created)
          __err = fclose(_M_cfile);
        _M_cfile = 0;
        if (!__err)
          __ret = this;
      }
    return __ret;
  }

_M_cfile_created is only true when it's opened using a filename (because it
calls fopen) or a file descriptor (because it calls fdopen).

[Bug libstdc++/98001] ext/stdio_filebuf/char/79820.cc is broken

[cvs-commit at gcc dot gnu.org]

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

--- Comment #5 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:2762cb1df686fc1ebcee23c7c4f0f6e8bf5a6abc

commit r11-5437-g2762cb1df686fc1ebcee23c7c4f0f6e8bf5a6abc
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Nov 26 11:25:55 2020 +0000

    libstdc++: Fix undefined FILE* operations in test

    We only need to check that the constructor doesn't clear errno, so
    there's no need to use an invalid FILE* for that.

    libstdc++-v3/ChangeLog:

            PR libstdc++/98001
            * testsuite/ext/stdio_filebuf/char/79820.cc: Do not pass invalid
            FILE* to constructor.

[Bug libstdc++/98001] ext/stdio_filebuf/char/79820.cc is broken

[redi at gcc dot gnu.org]

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

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I added b.close() before the fclose anyway. I'll backport this.

[Bug libstdc++/98001] ext/stdio_filebuf/char/79820.cc is broken

[cvs-commit at gcc dot gnu.org]

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

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

https://gcc.gnu.org/g:4cdc67405842946e08e7ddf375e850331530abb7

commit r10-9087-g4cdc67405842946e08e7ddf375e850331530abb7
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Nov 26 11:25:55 2020 +0000

    libstdc++: Fix undefined FILE* operations in test

    We only need to check that the constructor doesn't clear errno, so
    there's no need to use an invalid FILE* for that.

    libstdc++-v3/ChangeLog:

            PR libstdc++/98001
            * testsuite/ext/stdio_filebuf/char/79820.cc: Do not pass invalid
            FILE* to constructor.

    (cherry picked from commit 2762cb1df686fc1ebcee23c7c4f0f6e8bf5a6abc)

[Bug libstdc++/98001] ext/stdio_filebuf/char/79820.cc is broken

[cvs-commit at gcc dot gnu.org]

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

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

https://gcc.gnu.org/g:e45e65016754cf4bfc6c00cbbdca700f01f7c324

commit r9-9074-ge45e65016754cf4bfc6c00cbbdca700f01f7c324
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Nov 26 11:25:55 2020 +0000

    libstdc++: Fix undefined FILE* operations in test

    We only need to check that the constructor doesn't clear errno, so
    there's no need to use an invalid FILE* for that.

    libstdc++-v3/ChangeLog:

            PR libstdc++/98001
            * testsuite/ext/stdio_filebuf/char/79820.cc: Do not pass invalid
            FILE* to constructor.

    (cherry picked from commit 2762cb1df686fc1ebcee23c7c4f0f6e8bf5a6abc)

[Bug libstdc++/98001] ext/stdio_filebuf/char/79820.cc is broken

[cvs-commit at gcc dot gnu.org]

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

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

https://gcc.gnu.org/g:c6145860aac6acfeed2a98fe7532dd2cd0ffab2b

commit r8-10650-gc6145860aac6acfeed2a98fe7532dd2cd0ffab2b
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Nov 26 11:25:55 2020 +0000

    libstdc++: Fix undefined FILE* operations in test

    We only need to check that the constructor doesn't clear errno, so
    there's no need to use an invalid FILE* for that.

    libstdc++-v3/ChangeLog:

            PR libstdc++/98001
            * testsuite/ext/stdio_filebuf/char/79820.cc: Do not pass invalid
            FILE* to constructor.

    (cherry picked from commit 2762cb1df686fc1ebcee23c7c4f0f6e8bf5a6abc)

[Bug libstdc++/98001] ext/stdio_filebuf/char/79820.cc is broken

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
   Target Milestone|---                         |8.5
             Status|ASSIGNED                    |RESOLVED

--- Comment #10 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Fixed on all branches.