[v2] Add a new fwrite test for read-only streams

  From: Tulio Magno Quites Machado Filho <tuliom@redhat.com>

Changes since v1:

- Rebased the commit on top of the latest code;
- Removed implicit checks when using TEST_VERIFY_EXIT ();
- Fixed check for return code from create_temp_file ();
- Added a check that guarantees the file was not modified;

-- >8 --

Ensure that fwrite() behaves correctly even when the stream is
read-only.
---
 stdio-common/Makefile        |  1 +
 stdio-common/tst-fwrite-ro.c | 69 ++++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+)
 create mode 100644 stdio-common/tst-fwrite-ro.c
  

On 9/5/24 2:34 PM, Tulio Magno Quites Machado Filho wrote:
> From: Tulio Magno Quites Machado Filho <tuliom@redhat.com>
> 
> Changes since v1:
> 
> - Rebased the commit on top of the latest code;
> - Removed implicit checks when using TEST_VERIFY_EXIT ();
> - Fixed check for return code from create_temp_file ();
> - Added a check that guarantees the file was not modified;

Passed pre-commit CI so that's good.

Fixes Adhemerval's review comments.

LGTM.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> 
> -- >8 --
> 
> Ensure that fwrite() behaves correctly even when the stream is
> read-only.
> ---
>  stdio-common/Makefile        |  1 +
>  stdio-common/tst-fwrite-ro.c | 69 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 70 insertions(+)
>  create mode 100644 stdio-common/tst-fwrite-ro.c
> 
> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
> index d99e0cbfeb..0e18045038 100644
> --- a/stdio-common/Makefile
> +++ b/stdio-common/Makefile
> @@ -223,6 +223,7 @@ tests := \
>    tst-freopen64-3 \
>    tst-fseek \
>    tst-fwrite \
> +  tst-fwrite-ro \

OK.

>    tst-getline \
>    tst-getline-enomem \
>    tst-gets \
> diff --git a/stdio-common/tst-fwrite-ro.c b/stdio-common/tst-fwrite-ro.c
> new file mode 100644
> index 0000000000..b15753d4c4
> --- /dev/null
> +++ b/stdio-common/tst-fwrite-ro.c
> @@ -0,0 +1,69 @@
> +/* Test fwrite on a read-only stream.

OK.

> +   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>
> +#include <support/temp_file.h>
> +#include <support/xstdio.h>
> +#include <support/xunistd.h>
> +
> +/* A small buffer size is enough to run this test.  */
> +#define BUFSIZE 4
> +
> +static int
> +do_test (void)
> +{
> +  int fd;
> +  FILE *f;
> +  char buffer[BUFSIZE];
> +
> +  /* Create a temporary file and open it in read-only mode.  */
> +  fd = create_temp_file ("tst-fwrite-ro", NULL);
> +  TEST_VERIFY_EXIT (fd != -1);

OK. Good, no implicit check.

> +  f = fdopen (fd, "r");

OK. Read only.

> +  TEST_VERIFY_EXIT (f != NULL);

OK. Good, no implicit check.

> +
> +  /* Try to write to the temporary file with nmemb = 0, then check that
> +     fwrite returns 0.  No errors are expected from this.  */
> +  TEST_COMPARE (fwrite ("a", 1, 0, f), 0);
> +  TEST_COMPARE (ferror (f), 0);
> +
> +  /* Try to write to the temporary file with size = 0, then check that
> +     fwrite returns 0.  No errors are expected from this.  */
> +  TEST_COMPARE (fwrite ("a", 0, 1, f), 0);
> +  TEST_COMPARE (ferror (f), 0);
> +
> +  /* Try to write a single byte to the temporary file, then check that
> +     fwrite returns 0.  Check if an error was reported.  */
> +  TEST_COMPARE (fwrite ("a", 1, 1, f), 0);
> +  TEST_COMPARE (ferror (f), 1);
> +  clearerr (f);
> +
> +  /* Ensure that we are at the beginning of the file.  */
> +  rewind (f);
> +  /* Check if the file was not modified.  That means there is nothing to be
> +     read.  EOF must be reached.  */
> +  TEST_COMPARE (fread(buffer, 1, BUFSIZE, f), 0);
> +  TEST_COMPARE (feof (f), 1);

OK. Agreed, for a read-only empty buffer it must set EOF right away after reading nothing.

> +
> +  xfclose (f);
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
  

On 05/09/24 15:34, Tulio Magno Quites Machado Filho wrote:
> From: Tulio Magno Quites Machado Filho <tuliom@redhat.com>
> 
> Changes since v1:
> 
> - Rebased the commit on top of the latest code;
> - Removed implicit checks when using TEST_VERIFY_EXIT ();
> - Fixed check for return code from create_temp_file ();
> - Added a check that guarantees the file was not modified;
> 
> -- >8 --
> 
> Ensure that fwrite() behaves correctly even when the stream is
> read-only.

Looks good to me, just a suggestion below.

> ---
>  stdio-common/Makefile        |  1 +
>  stdio-common/tst-fwrite-ro.c | 69 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 70 insertions(+)
>  create mode 100644 stdio-common/tst-fwrite-ro.c
> 
> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
> index d99e0cbfeb..0e18045038 100644
> --- a/stdio-common/Makefile
> +++ b/stdio-common/Makefile
> @@ -223,6 +223,7 @@ tests := \
>    tst-freopen64-3 \
>    tst-fseek \
>    tst-fwrite \
> +  tst-fwrite-ro \
>    tst-getline \
>    tst-getline-enomem \
>    tst-gets \
> diff --git a/stdio-common/tst-fwrite-ro.c b/stdio-common/tst-fwrite-ro.c
> new file mode 100644
> index 0000000000..b15753d4c4
> --- /dev/null
> +++ b/stdio-common/tst-fwrite-ro.c
> @@ -0,0 +1,69 @@
> +/* Test fwrite on a read-only stream.
> +   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>
> +#include <support/temp_file.h>
> +#include <support/xstdio.h>
> +#include <support/xunistd.h>
> +
> +/* A small buffer size is enough to run this test.  */
> +#define BUFSIZE 4
> +
> +static int
> +do_test (void)
> +{
> +  int fd;
> +  FILE *f;
> +  char buffer[BUFSIZE];
> +
> +  /* Create a temporary file and open it in read-only mode.  */
> +  fd = create_temp_file ("tst-fwrite-ro", NULL);
> +  TEST_VERIFY_EXIT (fd != -1);
> +  f = fdopen (fd, "r");
> +  TEST_VERIFY_EXIT (f != NULL);
> +
> +  /* Try to write to the temporary file with nmemb = 0, then check that
> +     fwrite returns 0.  No errors are expected from this.  */
> +  TEST_COMPARE (fwrite ("a", 1, 0, f), 0);
> +  TEST_COMPARE (ferror (f), 0);
> +
> +  /* Try to write to the temporary file with size = 0, then check that
> +     fwrite returns 0.  No errors are expected from this.  */
> +  TEST_COMPARE (fwrite ("a", 0, 1, f), 0);
> +  TEST_COMPARE (ferror (f), 0);
> +
> +  /* Try to write a single byte to the temporary file, then check that
> +     fwrite returns 0.  Check if an error was reported.  */
> +  TEST_COMPARE (fwrite ("a", 1, 1, f), 0);
> +  TEST_COMPARE (ferror (f), 1);
> +  clearerr (f);
> +
> +  /* Ensure that we are at the beginning of the file.  */
> +  rewind (f);
> +  /* Check if the file was not modified.  That means there is nothing to be
> +     read.  EOF must be reached.  */
> +  TEST_COMPARE (fread(buffer, 1, BUFSIZE, f), 0);

Space before function name.  

Also, I would check if the file is empty by using syscall directly, to just 
check that stdio is not messing anything:

  {
    struct stat64 st;
    xfstat64 (fd, &st);
    TEST_COMPARE (st.st_size, 0);
  }

> +  TEST_COMPARE (feof (f), 1);
> +
> +  xfclose (f);
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
  

Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> writes:

> On 05/09/24 15:34, Tulio Magno Quites Machado Filho wrote:
>> +  /* Ensure that we are at the beginning of the file.  */
>> +  rewind (f);
>> +  /* Check if the file was not modified.  That means there is nothing to be
>> +     read.  EOF must be reached.  */
>> +  TEST_COMPARE (fread(buffer, 1, BUFSIZE, f), 0);
>
> Space before function name.  
>
> Also, I would check if the file is empty by using syscall directly, to just 
> check that stdio is not messing anything:
>
>   {
>     struct stat64 st;
>     xfstat64 (fd, &st);
>     TEST_COMPARE (st.st_size, 0);
>   }

Good suggestion. I replaced the previous 6 lines and the declaration of
buffer with your suggestion.

Pushed as 5d4ab106d4cf7d6e410d6fc3d460b090c9108682.
  

On Thu, 5 Sep 2024, Tulio Magno Quites Machado Filho wrote:

> diff --git a/stdio-common/tst-fwrite-ro.c b/stdio-common/tst-fwrite-ro.c
> new file mode 100644
> index 0000000000..b15753d4c4
> --- /dev/null
> +++ b/stdio-common/tst-fwrite-ro.c
[...]
> +  /* Try to write to the temporary file with nmemb = 0, then check that
> +     fwrite returns 0.  No errors are expected from this.  */
> +  TEST_COMPARE (fwrite ("a", 1, 0, f), 0);
> +  TEST_COMPARE (ferror (f), 0);

 Is it not a compliance bug though?  ISO C has this:

"The fwrite function returns the number of elements successfully written, 
which will be less than nmemb only if a write error is encountered.  If 
size or nmemb is zero, fwrite returns zero and the state of the stream 
remains unchanged."

which I interpret as a requirement for the call to return successfully 
only if the number of elements written matches the number requested (which 
in this case does not).

 Then POSIX Issue 8 further clarifies it:

"The fwrite() function shall return the number of elements successfully 
written, which shall be less than nitems only if a write error is 
encountered.  If size or nitems is 0, fwrite() shall return 0 and the 
state of the stream remains unchanged.  Otherwise, if a write error 
occurs, the error indicator for the stream shall be set, and errno shall 
be set to indicate the error."

which seems to agree with my interpretation in that we're supposed to set 
the stream's error indicator and errno accordingly here.

  Maciej
  

On Tue, 10 Sep 2024, Maciej W. Rozycki wrote:

> > diff --git a/stdio-common/tst-fwrite-ro.c b/stdio-common/tst-fwrite-ro.c
> > new file mode 100644
> > index 0000000000..b15753d4c4
> > --- /dev/null
> > +++ b/stdio-common/tst-fwrite-ro.c
> [...]
> > +  /* Try to write to the temporary file with nmemb = 0, then check that
> > +     fwrite returns 0.  No errors are expected from this.  */
> > +  TEST_COMPARE (fwrite ("a", 1, 0, f), 0);
> > +  TEST_COMPARE (ferror (f), 0);
> 
>  Is it not a compliance bug though?  ISO C has this:
> 
> "The fwrite function returns the number of elements successfully written, 
> which will be less than nmemb only if a write error is encountered.  If 
> size or nmemb is zero, fwrite returns zero and the state of the stream 
> remains unchanged."

 Not my day obviously, as nmemb is zero here.  Please consider my concern 
withdrawn.  Sorry for the confusion.

  Maciej