[v3,6/7] Use PRINTF_FORTIFY instead of _IO_FLAGS2_FORTIFY (bug 11319)

  On Tue, 04 Dec 2018, Adhemerval Zanella wrote:

>On 04/12/2018 16:01, Gabriel F. T. Gomes wrote:
>>
>> +#ifndef _GNU_SOURCE
>> +# define _GNU_SOURCE	1
>> +#endif  
>
>I think we can define it regardless

OK.

>> +#include <errno.h>
>> +#include <stdio.h>
>> +#include <string.h>
>> +#include <sys/eventfd.h>  
>
>eventfd is Linux specific, so either we need to move this test to Linux
>sysdeps or use a platform neutral way to stress this issue.

Oh, indeed.

>> +
>> +#include <support/check.h>
>> +
>> +static int
>> +do_test (void)
>> +{
>> +  int ret;
>> +  int fd = eventfd(0, 0);  
>
>To stress this issue we just need a valid file descriptor that is seekable
>but fail on a write.  Just use a temporary file opened in read-only mode.

Does the following patch look better (thanks for the suggestion)?

From d992bc941921988fee31ff8b3fc13fb406f89b93 Mon Sep 17 00:00:00 2001
From: "Gabriel F. T. Gomes" <gabriel@inconstante.eti.br>
Date: Tue, 4 Dec 2018 15:57:57 -0200
Subject: [PATCH v2] Add test for bug 11319

The commit

commit 7ca890b88e6ab7624afb1742a9fffb37ad5b3fc3
Author: Ulrich Drepper <drepper@redhat.com>
Date:   Wed Feb 24 16:07:57 2010 -0800

    Fix reporting of I/O errors in *dprintf functions.

fixed bug 11319 for dprintf and vdprintf, however, it did not fix it for
__dprintf_chk and __vdprintf_chk.  As a side-effect of the refactoring
of libio functions, this bug is also fixed for the foritified functions.
This patch adds a test case to avoid regressions.

Tested for powerpc64le and x86_64.

	* stdio-common/Makefile (tests): Add tst-bz11319 and
	tst-bz11319-fortify2.
	(CFLAGS-tst-bz11319-fortify2.c): New macro.
	* stdio-common/tst-bz11319-fortify2.c: New file.
	* stdio-common/tst-bz11319.c: Likewise.
---
 stdio-common/Makefile               |  6 ++++-
 stdio-common/tst-bz11319-fortify2.c |  1 +
 stdio-common/tst-bz11319.c          | 52 +++++++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+), 1 deletion(-)
 create mode 100644 stdio-common/tst-bz11319-fortify2.c
 create mode 100644 stdio-common/tst-bz11319.c
  

On 05/12/2018 14:53, Gabriel F. T. Gomes wrote:
> On Tue, 04 Dec 2018, Adhemerval Zanella wrote:
> 
>> On 04/12/2018 16:01, Gabriel F. T. Gomes wrote:
>>>
>>> +#ifndef _GNU_SOURCE
>>> +# define _GNU_SOURCE	1
>>> +#endif  
>>
>> I think we can define it regardless
> 
> OK.
> 
>>> +#include <errno.h>
>>> +#include <stdio.h>
>>> +#include <string.h>
>>> +#include <sys/eventfd.h>  
>>
>> eventfd is Linux specific, so either we need to move this test to Linux
>> sysdeps or use a platform neutral way to stress this issue.
> 
> Oh, indeed.
> 
>>> +
>>> +#include <support/check.h>
>>> +
>>> +static int
>>> +do_test (void)
>>> +{
>>> +  int ret;
>>> +  int fd = eventfd(0, 0);  
>>
>> To stress this issue we just need a valid file descriptor that is seekable
>> but fail on a write.  Just use a temporary file opened in read-only mode.
> 
> Does the following patch look better (thanks for the suggestion)?
> 
> From d992bc941921988fee31ff8b3fc13fb406f89b93 Mon Sep 17 00:00:00 2001
> From: "Gabriel F. T. Gomes" <gabriel@inconstante.eti.br>
> Date: Tue, 4 Dec 2018 15:57:57 -0200
> Subject: [PATCH v2] Add test for bug 11319
> 
> The commit
> 
> commit 7ca890b88e6ab7624afb1742a9fffb37ad5b3fc3
> Author: Ulrich Drepper <drepper@redhat.com>
> Date:   Wed Feb 24 16:07:57 2010 -0800
> 
>     Fix reporting of I/O errors in *dprintf functions.
> 
> fixed bug 11319 for dprintf and vdprintf, however, it did not fix it for
> __dprintf_chk and __vdprintf_chk.  As a side-effect of the refactoring
> of libio functions, this bug is also fixed for the foritified functions.
> This patch adds a test case to avoid regressions.
> 
> Tested for powerpc64le and x86_64.
> 
> 	* stdio-common/Makefile (tests): Add tst-bz11319 and
> 	tst-bz11319-fortify2.
> 	(CFLAGS-tst-bz11319-fortify2.c): New macro.
> 	* stdio-common/tst-bz11319-fortify2.c: New file.
> 	* stdio-common/tst-bz11319.c: Likewise.
> ---
>  stdio-common/Makefile               |  6 ++++-
>  stdio-common/tst-bz11319-fortify2.c |  1 +
>  stdio-common/tst-bz11319.c          | 52 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 58 insertions(+), 1 deletion(-)
>  create mode 100644 stdio-common/tst-bz11319-fortify2.c
>  create mode 100644 stdio-common/tst-bz11319.c
> 
> diff --git a/stdio-common/Makefile b/stdio-common/Makefile
> index 84bad1fafe..8978b3fb1f 100644
> --- a/stdio-common/Makefile
> +++ b/stdio-common/Makefile
> @@ -64,7 +64,7 @@ tests := tstscanf test_rdwr test-popen tstgetln test-fseek \
>  	 tst-vfprintf-user-type \
>  	 tst-vfprintf-mbs-prec \
>  	 tst-scanf-round \
> -	 tst-renameat2 \
> +	 tst-renameat2 tst-bz11319 tst-bz11319-fortify2 \
>  
>  test-srcs = tst-unbputc tst-printf tst-printfsz-islongdouble
>  
> @@ -164,6 +164,10 @@ CFLAGS-test_rdwr.c += -DOBJPFX=\"$(objpfx)\"
>  # tst-gets.c tests a deprecated function.
>  CFLAGS-tst-gets.c += -Wno-deprecated-declarations
>  
> +# BZ #11319 was first fixed for regular vdprintf, then reopened because
> +# the fortified version had the same bug.
> +CFLAGS-tst-bz11319-fortify2.c += -D_FORTIFY_SOURCE=2
> +
>  CPPFLAGS += $(libio-mtsafe)
>  
>  $(objpfx)tst-setvbuf1.out: /dev/null $(objpfx)tst-setvbuf1
> diff --git a/stdio-common/tst-bz11319-fortify2.c b/stdio-common/tst-bz11319-fortify2.c
> new file mode 100644
> index 0000000000..a8df9a39bd
> --- /dev/null
> +++ b/stdio-common/tst-bz11319-fortify2.c
> @@ -0,0 +1 @@
> +#include <tst-bz11319.c>
> diff --git a/stdio-common/tst-bz11319.c b/stdio-common/tst-bz11319.c
> new file mode 100644
> index 0000000000..29fbd729ab
> --- /dev/null
> +++ b/stdio-common/tst-bz11319.c
> @@ -0,0 +1,52 @@
> +/* Regression test for bug 11319.
> +   Copyright (C) 2018 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
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#define _GNU_SOURCE 1
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +
> +#include <support/check.h>
> +#include <support/temp_file.h>
> +#include <support/xstdio.h>
> +
> +static int
> +do_test (void)
> +{
> +  char *tempfile;
> +  int ret;
> +  FILE *fp;
> +
> +  /* Create a temporary file and open it in read-only mode.  */
> +  TEST_VERIFY_EXIT (create_temp_file ("tst-bz11319", &tempfile));
> +  fp = xfopen (tempfile, "r");
> +
> +  /* Try and write to the temporary file to intentionally fail, then
> +     check that dprintf (or __dprintf_chk) return EOF.  */
> +  ret = dprintf (fp->_fileno, "%d", 0);
> +  TEST_VERIFY (ret == EOF);
> +
> +  /* Clean up.  */
> +  xfclose (fp);
> +  if (tempfile)
> +    free (tempfile);

For a lack of a better interface (to create/open read-only temporary files
or memfd_create), I think it would be simpler to:

static int
do_test (void)
{ 
  char *tempfile;
  int fd;

  /* Create a temporary file and open it in read-only mode.  */
  TEST_VERIFY_EXIT (create_temp_file ("tst-bz11319", &tempfile));
  fd = xopen (tempfile, O_RDONLY, 0660);

  /* Try and write to the temporary file to intentionally fail, then
     check that dprintf (or __dprintf_chk) return EOF.  */
  TEST_COMPARE (dprintf (fd, "%d", 0), EOF);

  xclose (fd);
  free (tempfile);

  return 0;
}


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

On Wed, 05 Dec 2018, Adhemerval Zanella wrote:
>
>For a lack of a better interface (to create/open read-only temporary files
>or memfd_create), I think it would be simpler to:
>
>static int
>do_test (void)
>{ 
>  char *tempfile;
>  int fd;
>
>  /* Create a temporary file and open it in read-only mode.  */
>  TEST_VERIFY_EXIT (create_temp_file ("tst-bz11319", &tempfile));
>  fd = xopen (tempfile, O_RDONLY, 0660);
>
>  /* Try and write to the temporary file to intentionally fail, then
>     check that dprintf (or __dprintf_chk) return EOF.  */
>  TEST_COMPARE (dprintf (fd, "%d", 0), EOF);
>
>  xclose (fd);
>  free (tempfile);
>
>  return 0;
>}

Looks good to me.  Should I post an updated version, or is it OK with the
suggested changes?
  

On 05/12/2018 17:11, Gabriel F. T. Gomes wrote:
> On Wed, 05 Dec 2018, Adhemerval Zanella wrote:
>>
>> For a lack of a better interface (to create/open read-only temporary files
>> or memfd_create), I think it would be simpler to:
>>
>> static int
>> do_test (void)
>> { 
>>  char *tempfile;
>>  int fd;
>>
>>  /* Create a temporary file and open it in read-only mode.  */
>>  TEST_VERIFY_EXIT (create_temp_file ("tst-bz11319", &tempfile));
>>  fd = xopen (tempfile, O_RDONLY, 0660);
>>
>>  /* Try and write to the temporary file to intentionally fail, then
>>     check that dprintf (or __dprintf_chk) return EOF.  */
>>  TEST_COMPARE (dprintf (fd, "%d", 0), EOF);
>>
>>  xclose (fd);
>>  free (tempfile);
>>
>>  return 0;
>> }
> 
> Looks good to me.  Should I post an updated version, or is it OK with the
> suggested changes?
> 

I would suggest you to integrate it on the patch which fixes BZ#11319
and from my side you don't need to send an update version.
  

On Wed, 05 Dec 2018, Adhemerval Zanella wrote:

>I would suggest you to integrate it on the patch which fixes BZ#11319
>and from my side you don't need to send an update version.

Oh, I see.  Then I'll send it again, because I need to update the commit
message and ChangeLog.
  

On 05/12/2018 17:27, Gabriel F. T. Gomes wrote:
> On Wed, 05 Dec 2018, Adhemerval Zanella wrote:
> 
>> I would suggest you to integrate it on the patch which fixes BZ#11319
>> and from my side you don't need to send an update version.
> 
> Oh, I see.  Then I'll send it again, because I need to update the commit
> message and ChangeLog.
> 

Alright.
  

On Wed, 05 Dec 2018, Adhemerval Zanella wrote:

>On 05/12/2018 17:27, Gabriel F. T. Gomes wrote:
>> On Wed, 05 Dec 2018, Adhemerval Zanella wrote:
>>   
>>> I would suggest you to integrate it on the patch which fixes BZ#11319
>>> and from my side you don't need to send an update version.  
>> 
>> Oh, I see.  Then I'll send it again, because I need to update the commit
>> message and ChangeLog.
>>   
>
>Alright.

Hrm, I thought I would have to rewrite the messages (and that I would be
doubtful about it), but it turned out to be mostly copy-and-paste.  So,
I'll go ahead and push the changes.

Thanks again and sorry about the noise. :)