This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v3 1/7] Add __vfscanf_internal and __vfwscanf_internal with flags arguments.


On Thu, 22 Nov 2018, Adhemerval Zanella wrote:

>On 15/11/2018 19:44, Gabriel F. T. Gomes wrote:
>> From: Zack Weinberg <zackw@panix.com>
>>
> 
>The patch looks ok with the exception of the part where it adapts some symbol
>to set _IO_FLAGS2_SCANF_STD on _flags2 *without* acquiring the lock previously.
>I also see the idea of second patch is also to get rid of such mechanism, so
>maybe one option is to combine both patches.
>
>> [...]
>>
>>  int
>>  __isoc99_sscanf (const char *s, const char *format, ...)
>>  {
>>    va_list arg;
>>    int done;
>> +  _IO_strfile sf;
>> +  FILE *f = _IO_strfile_read (&sf, s);
>> +  f->_flags2 |= _IO_FLAGS2_SCANF_STD;
>>  
>>    va_start (arg, format);
>> -  done = __isoc99_vsscanf (s, format, arg);
>> +  done = __vfscanf_internal (f, format, arg, 0);
>>    va_end (arg);
>>  
>>    return done;  
>
>I think it requires acquire/release the 'f' lock before manipulating the its _flags2.
>Wouldn't be more consistent to fold the second patch in this set, which replace
>_IO_FLAGS2_SCANF_STD by SCANF_ISOC99_A, to avoid add the code which would be just
>removed in a subsequent patch?

I have no problems with folding the two patches, however I did not
understand why the lock needs to be acquired/released.  In this function,
`sf' is a variable on the stack, so it cannot be accessed elsewhere by
another thread (compare that against the `stream' argument to
__isoc99_vfscanf, below, where I agree that the lock is needed).

Did I miss something?

>> diff --git a/stdio-common/isoc99_vfscanf.c
>> b/stdio-common/isoc99_vfscanf.c index b80e05f8db..c96ca831ae 100644
>> --- a/stdio-common/isoc99_vfscanf.c
>> +++ b/stdio-common/isoc99_vfscanf.c
>> @@ -27,7 +27,7 @@ __isoc99_vfscanf (FILE *stream, const char *format,
>> va_list args) 
>>    _IO_acquire_lock_clear_flags2 (stream);
>>    stream->_flags2 |= _IO_FLAGS2_SCANF_STD;
>> -  done = _IO_vfscanf (stream, format, args, NULL);
>> +  done = __vfscanf_internal (stream, format, args, 0);
>>    _IO_release_lock (stream);
>>    return done;
>>  }  

For reference, __isoc99_vfscanf lock acquire/release.

>>  int
>>  __isoc99_vsscanf (const char *string, const char *format, va_list args)
>>  {
>> -  int ret;
>>    _IO_strfile sf;
>> -#ifdef _IO_MTSAFE_IO
>> -  sf._sbf._f._lock = NULL;
>> -#endif
>> -  _IO_no_init (&sf._sbf._f, _IO_USER_LOCK, -1, NULL, NULL);
>> -  _IO_JUMPS (&sf._sbf) = &_IO_str_jumps;
>> -  _IO_str_init_static_internal (&sf, (char*)string, 0, NULL);
>> -  sf._sbf._f._flags2 |= _IO_FLAGS2_SCANF_STD;
>> -  ret = _IO_vfscanf (&sf._sbf._f, format, args, NULL);
>> -  return ret;
>> +  FILE *f = _IO_strfile_read (&sf, string);
>> +  f->_flags2 |= _IO_FLAGS2_SCANF_STD;
>> +  return __vfscanf_internal (f, format, args, 0);
>>  }
>>  libc_hidden_def (__isoc99_vsscanf)  
>
>As before I think it requires to acquire the file lock before setting
>_flags2.

Same as __isoc99_sscanf.

>>  __isoc99_swscanf (const wchar_t *s, const wchar_t *format, ...)
>>  {
>>    va_list arg;
>>    int done;
>> +  _IO_strfile sf;
>> +  struct _IO_wide_data wd;
>> +  FILE *f = _IO_strfile_readw (&sf, &wd, s);
>> +  f->_flags2 |= _IO_FLAGS2_SCANF_STD;
>>  
>>    va_start (arg, format);
>> -  done = __isoc99_vswscanf (s, format, arg);
>> +  done = __vfwscanf_internal (f, format, arg, 0);
>>    va_end (arg);
>>  
>>    return done;  
>
>As before I think it requires to acquire the file lock before setting
>_flags2.

Likewise.

>>  int
>>  __isoc99_vswscanf (const wchar_t *string, const wchar_t *format,
>> va_list args) {
>> -  int ret;
>>    _IO_strfile sf;
>>    struct _IO_wide_data wd;
>> -#ifdef _IO_MTSAFE_IO
>> -  sf._sbf._f._lock = NULL;
>> -#endif
>> -  _IO_no_init (&sf._sbf._f, _IO_USER_LOCK, 0, &wd, &_IO_wstr_jumps);
>> -  _IO_fwide (&sf._sbf._f, 1);
>> -  _IO_wstr_init_static (&sf._sbf._f, (wchar_t *)string, 0, NULL);
>> -  sf._sbf._f._flags2 |= _IO_FLAGS2_SCANF_STD;
>> -  ret = _IO_vfwscanf ((FILE *) &sf._sbf, format, args, NULL);
>> -  return ret;
>> +  FILE *f = _IO_strfile_readw (&sf, &wd, string);
>> +  f->_flags2 |= _IO_FLAGS2_SCANF_STD;
>> +  return __vfwscanf_internal (f, format, args, 0);
>>  }
>>  libc_hidden_def (__isoc99_vswscanf)  
>
>As before I think it requires to acquire the file lock before setting
>_flags2.

Likewise.


I'm willing to squash the first and second patches anyway...  I just want
to make sure I understand your concern.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]