[v2] ftell: seek to end only when there are unflushed bytes (BZ #17647)
diff mbox

Message ID 54819F98.5080102@linux.vnet.ibm.com
State New
Headers show

Commit Message

Adhemerval Zanella Dec. 5, 2014, 12:05 p.m. UTC
On 04-12-2014 15:42, Adhemerval Zanella wrote:
> On 04-12-2014 13:58, Carlos O'Donell wrote:
>> On 12/04/2014 08:16 AM, Adhemerval Zanella wrote:
>>>         ftell: fdopen (file, "a+"): old offset = 0, new offset = 30, offset after EOF = 36
>>>         ftell: fopen (file, "(null)"): 
>>> Program received signal SIGSEGV, Segmentation fault.
>>> _IO_new_file_fopen (fp=0x212a2620, filename=0x212a0010 "/tmp/tst-active-handler-tmp.oSQlBj", mode=0x0, is32not64=1) at fileops.c:261
>>> 261       switch (*mode)
>>> (gdb) bt
>>> #0  _IO_new_file_fopen (fp=0x212a2620, filename=0x212a0010 "/tmp/tst-active-handler-tmp.oSQlBj", mode=0x0, is32not64=1) at fileops.c:261
>>> #1  0x00003fffb7e7ee7c in __fopen_internal (filename=0x212a0010 "/tmp/tst-active-handler-tmp.oSQlBj", mode=0x0, is32=<optimized out>) at iofopen.c:86
>>> #2  0x00003fffb7e7ef14 in _IO_new_fopen (filename=<optimized out>, mode=<optimized out>) at iofopen.c:97
>>> #3  0x0000000010001f88 in ?? ()
>>> #4  0x000000001000323c in ?? ()
>>> #5  0x000000001000180c in ?? ()
>>> #6  0x00003fffb7e2436c in generic_start_main (main=0x10015178, argc=<optimized out>, argv=0x3fffffffeb30, auxvec=0x3fffffffece0, init=<optimized out>, rtld_fini=<optimized out>, 
>>>     stack_end=<optimized out>, fini=<optimized out>) at ../csu/libc-start.c:289
>>> #7  0x00003fffb7e24594 in __libc_start_main (argc=<optimized out>, argv=<optimized out>, ev=<optimized out>, auxvec=<optimized out>, rtld_fini=<optimized out>, stinfo=<optimized out>, 
>>>     stack_on_entry=<optimized out>) at ../sysdeps/unix/sysv/linux/powerpc/libc-start.c:80
>>> #8  0x0000000000000000 in ?? ()
>>>
>>> GLIBC built with GCC 4.9.3 / -O3 (I have not tested different compiler/flags). I tried to
>>> debug a little and seems the test_modes is bogus somehow.  The testcase also fails running
>>> on system GLIBC (2.17/FC19 in my environment).
>> We would have seen this in our internal ppc/ppc64 builds, but we didn't
>> which means it is likely compiler dependent.
>>
>> I assume that when you say FC19 you still mean a ppc64 FC19? Similar
>> compiler issue?
>>
>> I've kicked off a ppc64le build of master to see what I can see.
> I also thinks it is a codegen issue I am seeing, although valgrind did not report
> any meaningful issue besides the invalid memory access itself. I will check more
> today.

I just debug a little and my understanding is I am seeing a buffer overrun on the stack
allocated variable:

365           if (test_modes[i].fd_mode != O_WRONLY)
366             {
367               char tmpbuf[data_len];
368 
369               rewind (fp);
370 
371               while (fgets_func (tmpbuf, sizeof (tmpbuf), fp) && !feof (fp));

The 'data_len' is calculated with wsclen and allocated as 'char'. The subsequent fgetws
will then try to write at most 'data_len' wchar_t in a buffer with just data_len 'char'.
The following patch fix the issues I am seeing on powerpc64:



What do you think?

Comments

Siddhesh Poyarekar Dec. 5, 2014, 12:31 p.m. UTC | #1
On Fri, Dec 05, 2014 at 10:05:44AM -0200, Adhemerval Zanella wrote:
> The 'data_len' is calculated with wsclen and allocated as 'char'. The subsequent fgetws
> will then try to write at most 'data_len' wchar_t in a buffer with just data_len 'char'.
> The following patch fix the issues I am seeing on powerpc64:
> 
> diff --git a/libio/tst-ftell-active-handler.c b/libio/tst-ftell-active-handler.c
> index f69e169..44a4fac 100644
> --- a/libio/tst-ftell-active-handler.c
> +++ b/libio/tst-ftell-active-handler.c
> @@ -84,6 +84,7 @@ static const char *char_data = "abcdef";
>  static const wchar_t *wide_data = L"abcdef";
>  static size_t data_len;
>  static size_t file_len;
> +static size_t char_len;
>  
>  typedef int (*fputs_func_t) (const void *data, FILE *fp);
>  typedef void *(*fgets_func_t) (void *ws, int n, FILE *fp);
> @@ -364,11 +365,11 @@ do_ftell_test (const char *filename)
>              reading.  */
>           if (test_modes[i].fd_mode != O_WRONLY)
>             {
> -             char tmpbuf[data_len];
> +             char tmpbuf[data_len * char_len];
>  
>               rewind (fp);
>  
> -             while (fgets_func (tmpbuf, sizeof (tmpbuf), fp) && !feof (fp));
> +             while (fgets_func (tmpbuf, data_len, fp) && !feof (fp));
>  
>               write_ret = write (fd, data, data_len);
>               if (write_ret != data_len)
> @@ -656,6 +657,7 @@ do_test (void)
>    fgets_func = (fgets_func_t) fgets;
>    data = char_data;
>    data_len = strlen (char_data);
> +  char_len = sizeof (char);
>    ret |= do_one_test (filename);
>  
>    /* Truncate the file before repeating the tests in wide mode.  */
> @@ -678,6 +680,7 @@ do_test (void)
>    fgets_func = (fgets_func_t) fgetws;
>    data = wide_data;
>    data_len = wcslen (wide_data);
> +  char_len = sizeof (wchar_t);
>    ret |= do_one_test (filename);
>  
>    return ret;
> 
> 
> What do you think?
> 

Ah yes, thanks for debugging this.  Looks good to me.

Siddhesh
Adhemerval Zanella Dec. 5, 2014, 12:57 p.m. UTC | #2
On 05-12-2014 10:31, Siddhesh Poyarekar wrote:
> On Fri, Dec 05, 2014 at 10:05:44AM -0200, Adhemerval Zanella wrote:
>> The 'data_len' is calculated with wsclen and allocated as 'char'. The subsequent fgetws
>> will then try to write at most 'data_len' wchar_t in a buffer with just data_len 'char'.
>> The following patch fix the issues I am seeing on powerpc64:
>>
>> diff --git a/libio/tst-ftell-active-handler.c b/libio/tst-ftell-active-handler.c
>> index f69e169..44a4fac 100644
>> --- a/libio/tst-ftell-active-handler.c
>> +++ b/libio/tst-ftell-active-handler.c
>> @@ -84,6 +84,7 @@ static const char *char_data = "abcdef";
>>  static const wchar_t *wide_data = L"abcdef";
>>  static size_t data_len;
>>  static size_t file_len;
>> +static size_t char_len;
>>  
>>  typedef int (*fputs_func_t) (const void *data, FILE *fp);
>>  typedef void *(*fgets_func_t) (void *ws, int n, FILE *fp);
>> @@ -364,11 +365,11 @@ do_ftell_test (const char *filename)
>>              reading.  */
>>           if (test_modes[i].fd_mode != O_WRONLY)
>>             {
>> -             char tmpbuf[data_len];
>> +             char tmpbuf[data_len * char_len];
>>  
>>               rewind (fp);
>>  
>> -             while (fgets_func (tmpbuf, sizeof (tmpbuf), fp) && !feof (fp));
>> +             while (fgets_func (tmpbuf, data_len, fp) && !feof (fp));
>>  
>>               write_ret = write (fd, data, data_len);
>>               if (write_ret != data_len)
>> @@ -656,6 +657,7 @@ do_test (void)
>>    fgets_func = (fgets_func_t) fgets;
>>    data = char_data;
>>    data_len = strlen (char_data);
>> +  char_len = sizeof (char);
>>    ret |= do_one_test (filename);
>>  
>>    /* Truncate the file before repeating the tests in wide mode.  */
>> @@ -678,6 +680,7 @@ do_test (void)
>>    fgets_func = (fgets_func_t) fgetws;
>>    data = wide_data;
>>    data_len = wcslen (wide_data);
>> +  char_len = sizeof (wchar_t);
>>    ret |= do_one_test (filename);
>>  
>>    return ret;
>>
>>
>> What do you think?
>>
> Ah yes, thanks for debugging this.  Looks good to me.
>
> Siddhesh
Pushed.
Andreas Schwab Dec. 5, 2014, 4:23 p.m. UTC | #3
Adhemerval Zanella <azanella@linux.vnet.ibm.com> writes:

> @@ -364,11 +365,11 @@ do_ftell_test (const char *filename)
>              reading.  */
>           if (test_modes[i].fd_mode != O_WRONLY)
>             {
> -             char tmpbuf[data_len];
> +             char tmpbuf[data_len * char_len];

tmpbuf may not be aligned for wchar_t.

Andreas.
Adhemerval Zanella Dec. 5, 2014, 4:35 p.m. UTC | #4
On 05-12-2014 14:23, Andreas Schwab wrote:
> Adhemerval Zanella <azanella@linux.vnet.ibm.com> writes:
>
>> @@ -364,11 +365,11 @@ do_ftell_test (const char *filename)
>>              reading.  */
>>           if (test_modes[i].fd_mode != O_WRONLY)
>>             {
>> -             char tmpbuf[data_len];
>> +             char tmpbuf[data_len * char_len];
> tmpbuf may not be aligned for wchar_t.

But I can't see why this won't allocate enough space for further operations. 
Care to explain?

>
> Andreas.
>
Andreas Schwab Dec. 5, 2014, 5:11 p.m. UTC | #5
Adhemerval Zanella <azanella@linux.vnet.ibm.com> writes:

> On 05-12-2014 14:23, Andreas Schwab wrote:
>> Adhemerval Zanella <azanella@linux.vnet.ibm.com> writes:
>>
>>> @@ -364,11 +365,11 @@ do_ftell_test (const char *filename)
>>>              reading.  */
>>>           if (test_modes[i].fd_mode != O_WRONLY)
>>>             {
>>> -             char tmpbuf[data_len];
>>> +             char tmpbuf[data_len * char_len];
>> tmpbuf may not be aligned for wchar_t.
>
> But I can't see why this won't allocate enough space for further operations. 

Where did I say that?

> Care to explain?

Alignment and size are separate properties of an object.

Andreas.

Patch
diff mbox

diff --git a/libio/tst-ftell-active-handler.c b/libio/tst-ftell-active-handler.c
index f69e169..44a4fac 100644
--- a/libio/tst-ftell-active-handler.c
+++ b/libio/tst-ftell-active-handler.c
@@ -84,6 +84,7 @@  static const char *char_data = "abcdef";
 static const wchar_t *wide_data = L"abcdef";
 static size_t data_len;
 static size_t file_len;
+static size_t char_len;
 
 typedef int (*fputs_func_t) (const void *data, FILE *fp);
 typedef void *(*fgets_func_t) (void *ws, int n, FILE *fp);
@@ -364,11 +365,11 @@  do_ftell_test (const char *filename)
             reading.  */
          if (test_modes[i].fd_mode != O_WRONLY)
            {
-             char tmpbuf[data_len];
+             char tmpbuf[data_len * char_len];
 
              rewind (fp);
 
-             while (fgets_func (tmpbuf, sizeof (tmpbuf), fp) && !feof (fp));
+             while (fgets_func (tmpbuf, data_len, fp) && !feof (fp));
 
              write_ret = write (fd, data, data_len);
              if (write_ret != data_len)
@@ -656,6 +657,7 @@  do_test (void)
   fgets_func = (fgets_func_t) fgets;
   data = char_data;
   data_len = strlen (char_data);
+  char_len = sizeof (char);
   ret |= do_one_test (filename);
 
   /* Truncate the file before repeating the tests in wide mode.  */
@@ -678,6 +680,7 @@  do_test (void)
   fgets_func = (fgets_func_t) fgetws;
   data = wide_data;
   data_len = wcslen (wide_data);
+  char_len = sizeof (wchar_t);
   ret |= do_one_test (filename);
 
   return ret;