y2038 and INLINE_SYSCALL

y2038 and INLINE_SYSCALL

[Adhemerval Zanella]

Recently on y2038 stat support, I followed the current implementations 
and issued __NR_statx using INLINE_SYSCALL_CALL.  It simplifies the code 
a bit, since we can check the return code for failures and get the errno
value directly (without using macros).

However in some environment it triggered some subtle issues on glibc code
where it either assumes a successful stat won't change errno and/or does
not handle ENOSYS.  For instance on elf/dl-load.c:

1968       if (here_any && (err = errno) != ENOENT && err != EACCES)
1969         /* The file exists and is readable, but something went wrong.  */
1970         return -1;

The errno is accesses regardless the previous stat64 (at line 1917)
has failed or not.

Another issue is at locale/programs/localedef.c:

237   output_path  = construct_output_path (argv[remaining]);
238   if (output_path == NULL && ! no_archive)
239     error (4, errno, _("cannot create directory for output files"));
240   cannot_write_why = errno;

Where construct_output_path will assume a successful stat won't touch
errno (it is used on euidaccess).

There is still _IO_SYSSTAT where jumps directly to stat and will report
that errno changes when fputs returns EOF.

So the question is which is the better way to handle it: change the
y2038 support to use INTERNAL_SYSCALL and the errno macros to avoid
touching 'errno' or fix the internal stat usage to no assume that
'errno' won't change on a success call?

For former, that other implementation that internally uses
INTERNAL_SYSCALL to check for a potential syscall to later use
a fallback (faccessat.c for instance).  So I am more inclined to
fix the latter.

Thoughts?

Re: y2038 and INLINE_SYSCALL

[Carlos O'Donell]

On 10/13/20 6:34 PM, Adhemerval Zanella via Libc-alpha wrote:
> Recently on y2038 stat support, I followed the current implementations 
> and issued __NR_statx using INLINE_SYSCALL_CALL.  It simplifies the code 
> a bit, since we can check the return code for failures and get the errno
> value directly (without using macros).
> 
> However in some environment it triggered some subtle issues on glibc code
> where it either assumes a successful stat won't change errno and/or does
> not handle ENOSYS.  For instance on elf/dl-load.c:
> 
> 1968       if (here_any && (err = errno) != ENOENT && err != EACCES)
> 1969         /* The file exists and is readable, but something went wrong.  */
> 1970         return -1;
> 
> The errno is accesses regardless the previous stat64 (at line 1917)
> has failed or not.

This is a bug in dl-load.c.
 
> Another issue is at locale/programs/localedef.c:
> 
> 237   output_path  = construct_output_path (argv[remaining]);
> 238   if (output_path == NULL && ! no_archive)
> 239     error (4, errno, _("cannot create directory for output files"));
> 240   cannot_write_why = errno;
> 
> Where construct_output_path will assume a successful stat won't touch
> errno (it is used on euidaccess).

A successful stat shouldn't appear to touch errno. Internally it can
save and restore errno (like signal handlers are required to do).

The ugly part is what does error () do on failure to flush or write to
the requisite streams. I'd expect it to touch errno. So I think the above
code should save errno first.

> There is still _IO_SYSSTAT where jumps directly to stat and will report
> that errno changes when fputs returns EOF.

That's a problem.
 
> So the question is which is the better way to handle it: change the
> y2038 support to use INTERNAL_SYSCALL and the errno macros to avoid
> touching 'errno' or fix the internal stat usage to no assume that
> 'errno' won't change on a success call?

I think the code is correct in assuming errno won't be touched
on a successful call.

> For former, that other implementation that internally uses
> INTERNAL_SYSCALL to check for a potential syscall to later use
> a fallback (faccessat.c for instance).  So I am more inclined to
> fix the latter.
> 
> Thoughts?
 
I'm not quite sure what you mean by "fix the latter" at this point.

Could you please clarify your position?

-- 
Cheers,
Carlos.

Re: y2038 and INLINE_SYSCALL

[Andreas Schwab]

On Okt 13 2020, Carlos O'Donell via Libc-alpha wrote:

>> Another issue is at locale/programs/localedef.c:
>> 
>> 237   output_path  = construct_output_path (argv[remaining]);
>> 238   if (output_path == NULL && ! no_archive)
>> 239     error (4, errno, _("cannot create directory for output files"));
>> 240   cannot_write_why = errno;
>> 
>> Where construct_output_path will assume a successful stat won't touch
>> errno (it is used on euidaccess).
>
> A successful stat shouldn't appear to touch errno. Internally it can
> save and restore errno (like signal handlers are required to do).
>
> The ugly part is what does error () do on failure to flush or write to
> the requisite streams. I'd expect it to touch errno. So I think the above
> code should save errno first.

The call to error won't return, since it is called with a non-zero
status.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: y2038 and INLINE_SYSCALL

[Florian Weimer]

* Carlos O'Donell via Libc-alpha:

> On 10/13/20 6:34 PM, Adhemerval Zanella via Libc-alpha wrote:
>> Recently on y2038 stat support, I followed the current implementations 
>> and issued __NR_statx using INLINE_SYSCALL_CALL.  It simplifies the code 
>> a bit, since we can check the return code for failures and get the errno
>> value directly (without using macros).
>> 
>> However in some environment it triggered some subtle issues on glibc code
>> where it either assumes a successful stat won't change errno and/or does
>> not handle ENOSYS.  For instance on elf/dl-load.c:
>> 
>> 1968       if (here_any && (err = errno) != ENOENT && err != EACCES)
>> 1969         /* The file exists and is readable, but something went wrong.  */
>> 1970         return -1;
>> 
>> The errno is accesses regardless the previous stat64 (at line 1917)
>> has failed or not.
>
> This is a bug in dl-load.c.

Still we should minimize application impact from the Y2038 work, without
requiring kernel updates.  Changing errno needlessly runs counter to
that.

I'm looking at i386 in particular, where backwards compatibility is most
important.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

Re: y2038 and INLINE_SYSCALL

[Adhemerval Zanella]

On 14/10/2020 05:09, Andreas Schwab wrote:
> On Okt 13 2020, Carlos O'Donell via Libc-alpha wrote:
> 
>>> Another issue is at locale/programs/localedef.c:
>>>
>>> 237   output_path  = construct_output_path (argv[remaining]);
>>> 238   if (output_path == NULL && ! no_archive)
>>> 239     error (4, errno, _("cannot create directory for output files"));
>>> 240   cannot_write_why = errno;
>>>
>>> Where construct_output_path will assume a successful stat won't touch
>>> errno (it is used on euidaccess).
>>
>> A successful stat shouldn't appear to touch errno. Internally it can
>> save and restore errno (like signal handlers are required to do).
>>
>> The ugly part is what does error () do on failure to flush or write to
>> the requisite streams. I'd expect it to touch errno. So I think the above
>> code should save errno first.
> 
> The call to error won't return, since it is called with a non-zero
> status.
> 
> Andreas.
> 

The issue is fact 'cannot_write_why' will be set to a non 0 value on
success on system that does not provide statx and later on

301   if (recorded_error_count == 0 || force_output != 0)
302     {
303       if (cannot_write_why != 0)
304         record_error (4, cannot_write_why, _("\
305 cannot write output files to `%s'"), output_path ? : argv[remaining]);

It will output a failure wrongly.

Re: y2038 and INLINE_SYSCALL

[Adhemerval Zanella]

On 14/10/2020 00:44, Carlos O'Donell wrote:
> On 10/13/20 6:34 PM, Adhemerval Zanella via Libc-alpha wrote:
>> Recently on y2038 stat support, I followed the current implementations 
>> and issued __NR_statx using INLINE_SYSCALL_CALL.  It simplifies the code 
>> a bit, since we can check the return code for failures and get the errno
>> value directly (without using macros).
>>
>> However in some environment it triggered some subtle issues on glibc code
>> where it either assumes a successful stat won't change errno and/or does
>> not handle ENOSYS.  For instance on elf/dl-load.c:
>>
>> 1968       if (here_any && (err = errno) != ENOENT && err != EACCES)
>> 1969         /* The file exists and is readable, but something went wrong.  */
>> 1970         return -1;
>>
>> The errno is accesses regardless the previous stat64 (at line 1917)
>> has failed or not.
> 
> This is a bug in dl-load.c.>  
>> Another issue is at locale/programs/localedef.c:
>>
>> 237   output_path  = construct_output_path (argv[remaining]);
>> 238   if (output_path == NULL && ! no_archive)
>> 239     error (4, errno, _("cannot create directory for output files"));
>> 240   cannot_write_why = errno;
>>
>> Where construct_output_path will assume a successful stat won't touch
>> errno (it is used on euidaccess).
> 
> A successful stat shouldn't appear to touch errno. Internally it can
> save and restore errno (like signal handlers are required to do).
> 
> The ugly part is what does error () do on failure to flush or write to
> the requisite streams. I'd expect it to touch errno. So I think the above
> code should save errno first.
> 
>> There is still _IO_SYSSTAT where jumps directly to stat and will report
>> that errno changes when fputs returns EOF.
> 
> That's a problem.
>  
>> So the question is which is the better way to handle it: change the
>> y2038 support to use INTERNAL_SYSCALL and the errno macros to avoid
>> touching 'errno' or fix the internal stat usage to no assume that
>> 'errno' won't change on a success call?
> 
> I think the code is correct in assuming errno won't be touched
> on a successful call.

This is tricky assumption specially for stdio which issues multiple
syscall along with the idea that a successful call might still set
errno no a non-value.

In any case, changing the new stat implementation to use
INTERNAL_SYSCALL should mitigate it.

> 
>> For former, that other implementation that internally uses
>> INTERNAL_SYSCALL to check for a potential syscall to later use
>> a fallback (faccessat.c for instance).  So I am more inclined to
>> fix the latter.
>>
>> Thoughts?
>  
> I'm not quite sure what you mean by "fix the latter" at this point.
> 
> Could you please clarify your position?

Sorry for confusion, I meant that we can either use INTERNAL_SYSCALL
and handle errno only when the implementation actually returns *or* 
we can handle the code that wrongly assumes errno can't be set by a 
successful call.

The INTERNAL_SYSCALL should be straightforward for stat, but maybe we
still need to reevaluate other implementations that also uses 
INLINE_SYSCALL and issues another syscalls to avoid clobbering errno
as for successful calls. 

And I tend to agree with Florian we should minimize application impact,
so I will change the new stat implementation to use INTERNAL_SYSCALL
and send fixes for both dl-load.c and localedef.c