Test fclose on an unopened file.

  Add new file libio/tst-fclosed-unopened.c that tests whether fclose on
an unopened file returns EOF.

fclose returning EOF for unopened files is not part of the external
contract but there are dependancies on this behaviour.  For example,
gnulib's close_stdout in lib/closeout.c.

Tested for x86_64.

Signed-off-by: Aaron Merey <amerey@redhat.com>
---
 libio/Makefile              |  1 +
 libio/tst-fclose-unopened.c | 40 +++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)
 create mode 100644 libio/tst-fclose-unopened.c
  

On Aug 29 2024, Aaron Merey wrote:

> Add new file libio/tst-fclosed-unopened.c that tests whether fclose on
> an unopened file returns EOF.

That is only true for the three standard files.  For all other, it's a
use-after-free.
  

On Thu, Aug 29, 2024 at 1:09 PM Andreas Schwab <schwab@linux-m68k.org>
wrote:

> On Aug 29 2024, Aaron Merey wrote:
>
> > Add new file libio/tst-fclosed-unopened.c that tests whether fclose on
> > an unopened file returns EOF.
>
> That is only true for the three standard files.  For all other, it's a
> use-after-free.
>
>
We know it is invalid, why not do what CHECK_FILE does if debugging is
enabled ?
I just tried just that and it does not crash anymore.though setting errno
to EINVAL may not be right for this particular case.
  

On Thu, Aug 29, 2024 at 1:09 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
>
> On Aug 29 2024, Aaron Merey wrote:
>
> > Add new file libio/tst-fclosed-unopened.c that tests whether fclose on
> > an unopened file returns EOF.
>
> That is only true for the three standard files.  For all other, it's a
> use-after-free.

I added the following to the commit message to clarify that fclosing
unopened files only applies to the standard streams:

Calling fclose on unopened files normally causes a use-after-free bug,
however the standard streams are an exception since they are not
deallocated by fclose.

On Thu, Aug 29, 2024 at 5:15 PM Cristian Rodríguez
<cristian@rodriguez.im> wrote:
>
> On Thu, Aug 29, 2024 at 1:09 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
>>
>> That is only true for the three standard files.  For all other, it's a
>> use-after-free.
>>
> We know it is invalid, why not do what CHECK_FILE does if debugging is enabled ?
> I just tried just that and it does not crash anymore.though setting errno to EINVAL may not be right for this particular case.

EBADF is appropriate for a file descriptor that "refers to no open file". [1]

Aaron

[1] https://pubs.opengroup.org/onlinepubs/9799919799/functions/V2_chap02.html
  

* Aaron Merey:

> Add new file libio/tst-fclosed-unopened.c that tests whether fclose on
> an unopened file returns EOF.
>
> fclose returning EOF for unopened files is not part of the external
> contract but there are dependancies on this behaviour.  For example,
> gnulib's close_stdout in lib/closeout.c.
>
> Tested for x86_64.
>
> Signed-off-by: Aaron Merey <amerey@redhat.com>
> ---
>  libio/Makefile              |  1 +
>  libio/tst-fclose-unopened.c | 40 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 41 insertions(+)
>  create mode 100644 libio/tst-fclose-unopened.c
>
> diff --git a/libio/Makefile b/libio/Makefile
> index 6a507b67ea..59f3ee0b7c 100644
> --- a/libio/Makefile
> +++ b/libio/Makefile
> @@ -95,6 +95,7 @@ tests = \
>    tst-eof \
>    tst-ext \
>    tst-ext2 \
> +  tst-fclose-unopened \
>    tst-fdopen-seek-failure \
>    tst-fgetc-after-eof \
>    tst-fgetwc \
> diff --git a/libio/tst-fclose-unopened.c b/libio/tst-fclose-unopened.c
> new file mode 100644
> index 0000000000..2c7cea0b52
> --- /dev/null
> +++ b/libio/tst-fclose-unopened.c
> @@ -0,0 +1,40 @@
> +/* Test using fclose on an unopened file.
> +   Copyright (C) 2024 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <stdio.h>
> +#include <support/check.h>
> +
> +/* Verify that fclose on an unopened file returns EOF.  This is not part
> +   of the fclose external contract but there are dependancies on this
> +   behaviour.  */
> +
> +static int
> +do_test (void)
> +{
> +  TEST_VERIFY (fclose (stdin) == 0);
> +
> +  /* Attempt to close the unopened file and verify that EOF is returned.
> +     Calling fclose on a file twice normally causes a use-after-free bug,
> +     however the standard streams are an exception since they are not
> +     deallocated by fclose.  */
> +  TEST_VERIFY (fclose (stdin) == EOF);

Sorry, missed that you can use

  TEST_COMPARE (fclose (stdin), 0);
  TEST_COMPARE (fclose (stdin), EOF);

It will provide additional diagnostics on failure.

We should also add a second test with an
libio/tst-fclose-unopened2.input file: read something from it, and then
do the same double fclose.  This will cover the case where a buffer has
been allocated.  We can do this in a separate patch if you prefer.

Do you have commit access to the glibc project, or should I push the
revised version (with TEST_COMPARE) on your behalf?

Thanks,
Florian
  

Hi Florian,

On Thu, Sep 5, 2024 at 7:58 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> Sorry, missed that you can use
>
>   TEST_COMPARE (fclose (stdin), 0);
>   TEST_COMPARE (fclose (stdin), EOF);
>
> It will provide additional diagnostics on failure.
>
> We should also add a second test with an
> libio/tst-fclose-unopened2.input file: read something from it, and then
> do the same double fclose.  This will cover the case where a buffer has
> been allocated.  We can do this in a separate patch if you prefer.

Ok I will add this in a separate patch.

> Do you have commit access to the glibc project, or should I push the
> revised version (with TEST_COMPARE) on your behalf?

Please push the revised version. I don't have commit access but I will
request it.

Aaron
  

On Thu, Sep 5, 2024 at 10:00 AM Aaron Merey <amerey@redhat.com> wrote:
> On Thu, Sep 5, 2024 at 7:58 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> > Do you have commit access to the glibc project, or should I push the
> > revised version (with TEST_COMPARE) on your behalf?
>
> Please push the revised version. I don't have commit access but I will
> request it.

The approval was quick. I have pushed the patch.

Thanks,
Aaron
  

* Aaron Merey:

> On Thu, Sep 5, 2024 at 10:00 AM Aaron Merey <amerey@redhat.com> wrote:
>> On Thu, Sep 5, 2024 at 7:58 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> > Do you have commit access to the glibc project, or should I push the
>> > revised version (with TEST_COMPARE) on your behalf?
>>
>> Please push the revised version. I don't have commit access but I will
>> request it.
>
> The approval was quick. I have pushed the patch.

In the future, please wait for Reviewed-by:, and include it in your
commit.

Do you have special arrangement with Red Hat?  If your contribution is
copyrighted by Red Hat, you should not use Signed-off-by:, either.

Thanks,
Florian
  

On Thu, Sep 5, 2024 at 10:26 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Aaron Merey:
>
> > On Thu, Sep 5, 2024 at 10:00 AM Aaron Merey <amerey@redhat.com> wrote:
> >> On Thu, Sep 5, 2024 at 7:58 AM Florian Weimer <fweimer@redhat.com> wrote:
> >>
> >> > Do you have commit access to the glibc project, or should I push the
> >> > revised version (with TEST_COMPARE) on your behalf?
> >>
> >> Please push the revised version. I don't have commit access but I will
> >> request it.
> >
> > The approval was quick. I have pushed the patch.
>
> In the future, please wait for Reviewed-by:, and include it in your
> commit.
>
> Do you have special arrangement with Red Hat?  If your contribution is
> copyrighted by Red Hat, you should not use Signed-off-by:, either.

Sorry about that. I do not have any special arrangement with Red Hat.
Will include Reviewed-by and avoid Signed-off-by in the future.

Thanks,
Aaron