glibc strerrorname_np

glibc strerrorname_np

[Jonny Grant]

Hi Carlos
I was pleased to see you added strerrorname_np()

May I ask, I couldn't find the file your implementation is in - could you point it out to me in the glibc repository please?

I noticed on the man page it may return NULL, which is a shame, as then it means we always need to check that before using in every printf etc :-

printf("err %s\n", strerrorname_np(myerr)?strerrorname_np(myerr), "Unknown err");
 
I'd done my own version a while ago as strerrno_s(), and assumed I could never get it accepted anywhere like glibc.
Probably I should have tried to submit it to glibc!
https://github.com/jonnygrant/safec/blob/master/strerrno.c

Would something like my implementation ever be accepted?
errno_t strerrno_s(char * const buf, const rsize_t buflen, const errno_t errnum)


Last quick question, do you know why strerror_r() is considered safer than strerror()? I guess someone could trash the memory returned by it?

char * errstr = strerror(EINVAL);
errstr[0] = '\0';  // trashed the process copy of the string. (or even SEGV it, if it was in some static section of the ELF?)

Kind regards
Jonny

Re: glibc strerrorname_np

[Adhemerval Zanella]

On 04/11/2021 17:23, Jonny Grant wrote:
> Hi Carlos
> I was pleased to see you added strerrorname_np()
> 
> May I ask, I couldn't find the file your implementation is in - could you point it out to me in the glibc repository please?

It is on string/strerrorname_np.c, which calls __get_errname() defined at
stdio-common/errlist.c.

> 
> I noticed on the man page it may return NULL, which is a shame, as then it means we always need to check that before using in every printf etc :-
> 
> printf("err %s\n", strerrorname_np(myerr)?strerrorname_np(myerr), "Unknown err");

I didn't considered printf() when I added strerrorname_np(). Maybe an empty string ("")
would be better than NULL.

>  
> I'd done my own version a while ago as strerrno_s(), and assumed I could never get it accepted anywhere like glibc.
> Probably I should have tried to submit it to glibc!
> https://github.com/jonnygrant/safec/blob/master/strerrno.c
> 
> Would something like my implementation ever be accepted?
> errno_t strerrno_s(char * const buf, const rsize_t buflen, const errno_t errnum)

So this is basically:

  int strerrno_s (char *buf, size_t buflen, int errnum)
  {
    const char *r = strerrorname_np (errnum);
    __snprintf (buf, buflen, "%s", r == NULL ? "" : r);
    return errnum;
  }

I don't see much gain on adding another wrapper to format errno, the idea of
strerrorname_np() was to provide a async-signal-safe way to map errno to
string (by avoiding translation).

> 
> 
> Last quick question, do you know why strerror_r() is considered safer than strerror()? I guess someone could trash the memory returned by it?
> 
> char * errstr = strerror(EINVAL);
> errstr[0] = '\0';  // trashed the process copy of the string. (or even SEGV it, if it was in some static section of the ELF?)

It is undefined-behavior when you modify the return value (both C and POSIX
are explicit about it). On glibc, both strerror() and strerror_l() returns
the same thread local buffer (so you might modify in a thread-safe manner). 

Re: glibc strerrorname_np

[Jonny Grant]

Hi Adhemerval, Thank you for you reply.

On 04/11/2021 20:46, Adhemerval Zanella wrote:
> 
> 
> On 04/11/2021 17:23, Jonny Grant wrote:
>> Hi Carlos
>> I was pleased to see you added strerrorname_np()
>>
>> May I ask, I couldn't find the file your implementation is in - could you point it out to me in the glibc repository please?
> 
> It is on string/strerrorname_np.c, which calls __get_errname() defined at
> stdio-common/errlist.c.

Ok yes, I see in glibc/stdio-common/errlist.c the __get_errname() implementation.

>> I noticed on the man page it may return NULL, which is a shame, as then it means we always need to check that before using in every printf etc :-
>>
>> printf("err %s\n", strerrorname_np(myerr)?strerrorname_np(myerr), "Unknown err");
> 
> I didn't considered printf() when I added strerrorname_np(). Maybe an empty string ("")
> would be better than NULL.

Yes, "" empty string sounds great.
 
>> I'd done my own version a while ago as strerrno_s(), and assumed I could never get it accepted anywhere like glibc.
>> Probably I should have tried to submit it to glibc!
>> https://github.com/jonnygrant/safec/blob/master/strerrno.c
>>
>> Would something like my implementation ever be accepted?
>> errno_t strerrno_s(char * const buf, const rsize_t buflen, const errno_t errnum)
> 
> So this is basically:
> 
>   int strerrno_s (char *buf, size_t buflen, int errnum)
>   {
>     const char *r = strerrorname_np (errnum);
>     __snprintf (buf, buflen, "%s", r == NULL ? "" : r);
>     return errnum;
>   }
> 
> I don't see much gain on adding another wrapper to format errno, the idea of
> strerrorname_np() was to provide a async-signal-safe way to map errno to
> string (by avoiding translation).

Yes, I should migrate to your strerrorname_np() version soon. If you could change it to return "" instead of NULL, that would be very much appreciated to avoid any accidental SEGV.

>> Last quick question, do you know why strerror_r() is considered safer than strerror()? I guess someone could trash the memory returned by it?
>>
>> char * errstr = strerror(EINVAL);
>> errstr[0] = '\0';  // trashed the process copy of the string. (or even SEGV it, if it was in some static section of the ELF?)
> 
> It is undefined-behavior when you modify the return value (both C and POSIX
> are explicit about it). On glibc, both strerror() and strerror_l() returns
> the same thread local buffer (so you might modify in a thread-safe manner). 


Thank you
Jonny

Re: glibc strerrorname_np

[Adhemerval Zanella]

On 04/11/2021 19:52, Jonny Grant wrote:
> Hi Adhemerval, Thank you for you reply.
> 
> On 04/11/2021 20:46, Adhemerval Zanella wrote:
>>
>>
>> On 04/11/2021 17:23, Jonny Grant wrote:
>>> Hi Carlos
>>> I was pleased to see you added strerrorname_np()
>>>
>>> May I ask, I couldn't find the file your implementation is in - could you point it out to me in the glibc repository please?
>>
>> It is on string/strerrorname_np.c, which calls __get_errname() defined at
>> stdio-common/errlist.c.
> 
> Ok yes, I see in glibc/stdio-common/errlist.c the __get_errname() implementation.
> 
>>> I noticed on the man page it may return NULL, which is a shame, as then it means we always need to check that before using in every printf etc :-
>>>
>>> printf("err %s\n", strerrorname_np(myerr)?strerrorname_np(myerr), "Unknown err");
>>
>> I didn't considered printf() when I added strerrorname_np(). Maybe an empty string ("")
>> would be better than NULL.
> 
> Yes, "" empty string sounds great.
>  
>>> I'd done my own version a while ago as strerrno_s(), and assumed I could never get it accepted anywhere like glibc.
>>> Probably I should have tried to submit it to glibc!
>>> https://github.com/jonnygrant/safec/blob/master/strerrno.c
>>>
>>> Would something like my implementation ever be accepted?
>>> errno_t strerrno_s(char * const buf, const rsize_t buflen, const errno_t errnum)
>>
>> So this is basically:
>>
>>   int strerrno_s (char *buf, size_t buflen, int errnum)
>>   {
>>     const char *r = strerrorname_np (errnum);
>>     __snprintf (buf, buflen, "%s", r == NULL ? "" : r);
>>     return errnum;
>>   }
>>
>> I don't see much gain on adding another wrapper to format errno, the idea of
>> strerrorname_np() was to provide a async-signal-safe way to map errno to
>> string (by avoiding translation).
> 
> Yes, I should migrate to your strerrorname_np() version soon. If you could change it to return "" instead of NULL, that would be very much appreciated to avoid any accidental SEGV.

This is most likely an ABI break and I really like to avoid it since it would
require an new version and another implementation for such symbol.

Re: glibc strerrorname_np

[Jonny Grant]

On 04/11/2021 23:28, Adhemerval Zanella wrote:
> 
> 
> On 04/11/2021 19:52, Jonny Grant wrote:
>> Hi Adhemerval, Thank you for you reply.
>>
>> On 04/11/2021 20:46, Adhemerval Zanella wrote:
>>>
>>>
>>> On 04/11/2021 17:23, Jonny Grant wrote:
>>>> Hi Carlos
>>>> I was pleased to see you added strerrorname_np()
>>>>
>>>> May I ask, I couldn't find the file your implementation is in - could you point it out to me in the glibc repository please?
>>>
>>> It is on string/strerrorname_np.c, which calls __get_errname() defined at
>>> stdio-common/errlist.c.
>>
>> Ok yes, I see in glibc/stdio-common/errlist.c the __get_errname() implementation.
>>
>>>> I noticed on the man page it may return NULL, which is a shame, as then it means we always need to check that before using in every printf etc :-
>>>>
>>>> printf("err %s\n", strerrorname_np(myerr)?strerrorname_np(myerr), "Unknown err");
>>>
>>> I didn't considered printf() when I added strerrorname_np(). Maybe an empty string ("")
>>> would be better than NULL.
>>
>> Yes, "" empty string sounds great.
>>  
>>>> I'd done my own version a while ago as strerrno_s(), and assumed I could never get it accepted anywhere like glibc.
>>>> Probably I should have tried to submit it to glibc!
>>>> https://github.com/jonnygrant/safec/blob/master/strerrno.c
>>>>
>>>> Would something like my implementation ever be accepted?
>>>> errno_t strerrno_s(char * const buf, const rsize_t buflen, const errno_t errnum)
>>>
>>> So this is basically:
>>>
>>>   int strerrno_s (char *buf, size_t buflen, int errnum)
>>>   {
>>>     const char *r = strerrorname_np (errnum);
>>>     __snprintf (buf, buflen, "%s", r == NULL ? "" : r);
>>>     return errnum;
>>>   }
>>>
>>> I don't see much gain on adding another wrapper to format errno, the idea of
>>> strerrorname_np() was to provide a async-signal-safe way to map errno to
>>> string (by avoiding translation).
>>
>> Yes, I should migrate to your strerrorname_np() version soon. If you could change it to return "" instead of NULL, that would be very much appreciated to avoid any accidental SEGV.
> 
> This is most likely an ABI break and I really like to avoid it since it would
> require an new version and another implementation for such symbol.
> 

Hi Adhemerval

Thank you for your reply. Personally I understood an ABI break would be the return type, the name, or the parameters. But the proposed change is not so. Changing to return a string, should be fine.

ie, in relation to strerror() C99 and POSIX.1-2008 require the return value to be non-NULL. (my view is it is always better not to return a NULL from such string functions that could then cause a SEGV.

strerror(1000) returns a string "Unknown error 1000"

Better to simply align with glibc strerror() approach?

Feels like there is still time to change it, as it is _np. Aligning with strerror(), or just "" as you had mentioned seems reasonable.


https://man7.org/linux/man-pages/man3/strerror.3.html

It's common for some returns to change, eg glibc 2.13 changed strerror_r() behaviour to return the actual error code, as opposed to returning -1 and setting errno.

Cheers
Jonny

Re: glibc strerrorname_np

[Adhemerval Zanella]

On 05/11/2021 08:51, Jonny Grant wrote:
>>
> 
> Hi Adhemerval
> 
> Thank you for your reply. Personally I understood an ABI break would be the return type, the name, or the parameters. But the proposed change is not so. Changing to return a string, should be fine.

It is still an ABI break, code that checks NULL for invalid input will
stop to work.

> 
> ie, in relation to strerror() C99 and POSIX.1-2008 require the return value to be non-NULL. (my view is it is always better not to return a NULL from such string functions that could then cause a SEGV.
> 
> strerror(1000) returns a string "Unknown error 1000"
> 
> Better to simply align with glibc strerror() approach?
> 
> Feels like there is still time to change it, as it is _np. Aligning with strerror(), or just "" as you had mentioned seems reasonable.

I give you that it is indeed a better return code, and it is not a matter
of timing, but rather I don't think it really worth the ABI break and 
the required code complexity to do so. 

It would require:

  1. Change the strerrorname_np to return "" on invalid code.
  2. Keep the compat symbol that returns NULL and add a compat symbol.
  3. Exports a new symbol with version on 2.35 with the new semantic
     and update the ailist.
  4. Update the documentation and sync with man-pages. 

> 
> 
> https://man7.org/linux/man-pages/man3/strerror.3.html
> 
> It's common for some returns to change, eg glibc 2.13 changed strerror_r() behaviour to return the actual error code, as opposed to returning -1 and setting errno.

And such change did got without burden and extra complexity. Just check
the multiple preprocessor checks it requires to get the right definition
depending of the system support on the misc/error.c (imported from gnulib).

Re: glibc strerrorname_np

[Jonny Grant]

[-- Attachment #1: Type: text/plain, Size: 2271 bytes --]



On 05/11/2021 13:01, Adhemerval Zanella wrote:
> 
> 
> On 05/11/2021 08:51, Jonny Grant wrote:
>>>
>>
>> Hi Adhemerval
>>
>> Thank you for your reply. Personally I understood an ABI break would be the return type, the name, or the parameters. But the proposed change is not so. Changing to return a string, should be fine.
> 
> It is still an ABI break, code that checks NULL for invalid input will
> stop to work.
> 
>>
>> ie, in relation to strerror() C99 and POSIX.1-2008 require the return value to be non-NULL. (my view is it is always better not to return a NULL from such string functions that could then cause a SEGV.
>>
>> strerror(1000) returns a string "Unknown error 1000"
>>
>> Better to simply align with glibc strerror() approach?
>>
>> Feels like there is still time to change it, as it is _np. Aligning with strerror(), or just "" as you had mentioned seems reasonable.
> 
> I give you that it is indeed a better return code, and it is not a matter
> of timing, but rather I don't think it really worth the ABI break and 
> the required code complexity to do so. 
> 
> It would require:
> 
>   1. Change the strerrorname_np to return "" on invalid code.

Please find attached the patch.

>   2. Keep the compat symbol that returns NULL and add a compat symbol.
>   3. Exports a new symbol with version on 2.35 with the new semantic
>      and update the ailist.

May I check, why would a new symbol be needed? I'd expect it is only a change to strerrorname_np and any test code you have that presently checks for NULL return.

>   4. Update the documentation and sync with man-pages. 

The man-page update is minor, I could handle that.


>> https://man7.org/linux/man-pages/man3/strerror.3.html
>>
>> It's common for some returns to change, eg glibc 2.13 changed strerror_r() behaviour to return the actual error code, as opposed to returning -1 and setting errno.
> 
> And such change did got without burden and extra complexity. Just check
> the multiple preprocessor checks it requires to get the right definition
> depending of the system support on the misc/error.c (imported from gnulib).
> 

Ok, I think the change I propose does not affect the definition, as the function signature is the same. Maybe I misunderstand something.

Cheers, Jonny

[-- Attachment #2: errlist.c.patch --]
[-- Type: text/x-patch, Size: 337 bytes --]

--- errlist.c.orig	2021-11-05 22:18:24.021988414 +0000
+++ errlist.c	2021-11-05 22:20:12.744347665 +0000
@@ -69,7 +69,7 @@
 {
   if (errnum < 0 || errnum >= array_length (_sys_errnameidx)
       || (errnum > 0 && _sys_errnameidx[errnum] == 0))
-    return NULL;
+    return "";
   return _sys_errname.str + _sys_errnameidx[errnum];
 }
 

Re: glibc strerrorname_np

[Adhemerval Zanella]

On 05/11/2021 19:23, Jonny Grant wrote:
> 
> 
> On 05/11/2021 13:01, Adhemerval Zanella wrote:
>>
>>
>> On 05/11/2021 08:51, Jonny Grant wrote:
>>>>
>>>
>>> Hi Adhemerval
>>>
>>> Thank you for your reply. Personally I understood an ABI break would be the return type, the name, or the parameters. But the proposed change is not so. Changing to return a string, should be fine.
>>
>> It is still an ABI break, code that checks NULL for invalid input will
>> stop to work.
>>
>>>
>>> ie, in relation to strerror() C99 and POSIX.1-2008 require the return value to be non-NULL. (my view is it is always better not to return a NULL from such string functions that could then cause a SEGV.
>>>
>>> strerror(1000) returns a string "Unknown error 1000"
>>>
>>> Better to simply align with glibc strerror() approach?
>>>
>>> Feels like there is still time to change it, as it is _np. Aligning with strerror(), or just "" as you had mentioned seems reasonable.
>>
>> I give you that it is indeed a better return code, and it is not a matter
>> of timing, but rather I don't think it really worth the ABI break and 
>> the required code complexity to do so. 
>>
>> It would require:
>>
>>   1. Change the strerrorname_np to return "" on invalid code.
> 
> Please find attached the patch.
> 
>>   2. Keep the compat symbol that returns NULL and add a compat symbol.
>>   3. Exports a new symbol with version on 2.35 with the new semantic
>>      and update the ailist.
> 
> May I check, why would a new symbol be needed? I'd expect it is only a change to strerrorname_np and any test code you have that presently checks for NULL return.

As I said before it is an ABI break, since users that check for invalid 
errno against NULL will start to fail.  For such change we *do need* all 
the trouble of adding a compat symbol with current semantic.

> 
>>   4. Update the documentation and sync with man-pages. 
> 
> The man-page update is minor, I could handle that.
> 
> 
>>> https://man7.org/linux/man-pages/man3/strerror.3.html
>>>
>>> It's common for some returns to change, eg glibc 2.13 changed strerror_r() behaviour to return the actual error code, as opposed to returning -1 and setting errno.
>>
>> And such change did got without burden and extra complexity. Just check
>> the multiple preprocessor checks it requires to get the right definition
>> depending of the system support on the misc/error.c (imported from gnulib).
>>
> 
> Ok, I think the change I propose does not affect the definition, as the function signature is the same. Maybe I misunderstand something.

The ABI break is not only for function signature and input alignment/size,
but also for function semantic.  Just check the fmemopen
(fdb7d390dd0d96e4a8239c46f3aa64598b90842b), where we kept the old buggy 
implementation since even when it is not POSIX compliant because we do
not know if users do depend of such behavior.

Re: glibc strerrorname_np

[Zack Weinberg]

On Sat, Nov 6, 2021, at 8:51 AM, Adhemerval Zanella via Libc-alpha wrote:
> On 05/11/2021 19:23, Jonny Grant wrote:
>> On 05/11/2021 13:01, Adhemerval Zanella wrote:
>>> On 05/11/2021 08:51, Jonny Grant wrote:
>>>> Thank you for your reply. Personally I understood an ABI break would be
>>>> the return type, the name, or the parameters. But the proposed change is
>>>> not so. Changing to return a string, should be fine.
>>>
>>> It is still an ABI break, code that checks NULL for invalid input will
>>> stop to work.
...
> As I said before it is an ABI break, since users that check for invalid 
> errno against NULL will start to fail.  For such change we *do need* all 
> the trouble of adding a compat symbol with current semantic.

A compat symbol doesn't do any good here, though.  As soon as the program is recompiled it will start getting the new semantics, and since that doesn't cause a compile-time error, the break will go unnoticed -- particularly in this case, since this is likely to affect only error-handling paths that test suites tend not to exercise.

If we want to preserve backward compatibility we need a whole new function name.

zw

Re: glibc strerrorname_np

[Adhemerval Zanella]

On 07/11/2021 14:37, Zack Weinberg via Libc-alpha wrote:
> On Sat, Nov 6, 2021, at 8:51 AM, Adhemerval Zanella via Libc-alpha wrote:
>> On 05/11/2021 19:23, Jonny Grant wrote:
>>> On 05/11/2021 13:01, Adhemerval Zanella wrote:
>>>> On 05/11/2021 08:51, Jonny Grant wrote:
>>>>> Thank you for your reply. Personally I understood an ABI break would be
>>>>> the return type, the name, or the parameters. But the proposed change is
>>>>> not so. Changing to return a string, should be fine.
>>>>
>>>> It is still an ABI break, code that checks NULL for invalid input will
>>>> stop to work.
> ...
>> As I said before it is an ABI break, since users that check for invalid 
>> errno against NULL will start to fail.  For such change we *do need* all 
>> the trouble of adding a compat symbol with current semantic.
> 
> A compat symbol doesn't do any good here, though.  As soon as the program is recompiled it will start getting the new semantics, and since that doesn't cause a compile-time error, the break will go unnoticed -- particularly in this case, since this is likely to affect only error-handling paths that test suites tend not to exercise.
> 
> If we want to preserve backward compatibility we need a whole new function name.

This is the burden of any semantic change on exported symbols: new users
will need to adapt to it, since the idea is to keep older programs 
expecting the new semantic (since either rebuilding is not possible or
troublesome).

Re: glibc strerrorname_np

[Zack Weinberg]

On Mon, Nov 8, 2021, at 8:56 AM, Adhemerval Zanella wrote:
> On 07/11/2021 14:37, Zack Weinberg via Libc-alpha wrote:
>> On Sat, Nov 6, 2021, at 8:51 AM, Adhemerval Zanella via Libc-alpha wrote:
>>> On 05/11/2021 19:23, Jonny Grant wrote:
>>>> On 05/11/2021 13:01, Adhemerval Zanella wrote:
>>>>> On 05/11/2021 08:51, Jonny Grant wrote:
>>>>>> Thank you for your reply. Personally I understood an ABI break would be
>>>>>> the return type, the name, or the parameters. But the proposed change is
>>>>>> not so. Changing to return a string, should be fine.
>>>>>
>>>>> It is still an ABI break, code that checks NULL for invalid input will
>>>>> stop to work.
>> ...
>>> As I said before it is an ABI break, since users that check for invalid 
>>> errno against NULL will start to fail.  For such change we *do need* all 
>>> the trouble of adding a compat symbol with current semantic.
>> 
>> A compat symbol doesn't do any good here, though.  As soon as the program is recompiled it will start getting the new semantics, and since that doesn't cause a compile-time error, the break will go unnoticed -- particularly in this case, since this is likely to affect only error-handling paths that test suites tend not to exercise.
>> 
>> If we want to preserve backward compatibility we need a whole new function name.
>
> This is the burden of any semantic change on exported symbols: new users
> will need to adapt to it, since the idea is to keep older programs 
> expecting the new semantic (since either rebuilding is not possible or
> troublesome).

I think you misunderstand.  I'm saying that a compat symbol is *not enough* backward compatibility, since it only protects old binaries, not old sources that have been recompiled with a newer libc.  Compat symbols, in my view, are only sufficient when either a recompilation will fix whatever the problem was (e.g. changing the size of a FILE) or when old code will not *compile* until corrected for the new semantics.

I know we have done compat symbols as the only backward compatibility net for runtime-only semantic changes in the past, but I think that was wrong and we shouldn't do it anymore.

zw

Re: glibc strerrorname_np

[Adhemerval Zanella]

On 08/11/2021 15:42, Zack Weinberg wrote:
> On Mon, Nov 8, 2021, at 8:56 AM, Adhemerval Zanella wrote:
>> On 07/11/2021 14:37, Zack Weinberg via Libc-alpha wrote:
>>> On Sat, Nov 6, 2021, at 8:51 AM, Adhemerval Zanella via Libc-alpha wrote:
>>>> On 05/11/2021 19:23, Jonny Grant wrote:
>>>>> On 05/11/2021 13:01, Adhemerval Zanella wrote:
>>>>>> On 05/11/2021 08:51, Jonny Grant wrote:
>>>>>>> Thank you for your reply. Personally I understood an ABI break would be
>>>>>>> the return type, the name, or the parameters. But the proposed change is
>>>>>>> not so. Changing to return a string, should be fine.
>>>>>>
>>>>>> It is still an ABI break, code that checks NULL for invalid input will
>>>>>> stop to work.
>>> ...
>>>> As I said before it is an ABI break, since users that check for invalid 
>>>> errno against NULL will start to fail.  For such change we *do need* all 
>>>> the trouble of adding a compat symbol with current semantic.
>>>
>>> A compat symbol doesn't do any good here, though.  As soon as the program is recompiled it will start getting the new semantics, and since that doesn't cause a compile-time error, the break will go unnoticed -- particularly in this case, since this is likely to affect only error-handling paths that test suites tend not to exercise.
>>>
>>> If we want to preserve backward compatibility we need a whole new function name.
>>
>> This is the burden of any semantic change on exported symbols: new users
>> will need to adapt to it, since the idea is to keep older programs 
>> expecting the new semantic (since either rebuilding is not possible or
>> troublesome).
> 
> I think you misunderstand.  I'm saying that a compat symbol is *not enough* backward compatibility, since it only protects old binaries, not old sources that have been recompiled with a newer libc.  Compat symbols, in my view, are only sufficient when either a recompilation will fix whatever the problem was (e.g. changing the size of a FILE) or when old code will not *compile* until corrected for the new semantics.
> 
> I know we have done compat symbols as the only backward compatibility net for runtime-only semantic changes in the past, but I think that was wrong and we shouldn't do it anymore.

Well, compat symbols is meant to keep old binaries running without introducing
regression that would require a redeploy, it is not meant to fix old sources
that might not rebuild against newer libc (since it is an GNU extension).
For such cases we do need some code fixes (as it is constantly done on
gnulib, for instance).

But I do agree with you that at least keeping old buggy behavior for the
sake of 'compatibility' is not a practice that I think help improve the
whole ecosystem as whole (although it does help close-source and/or binary
one system where recompile is an issue).

Re: glibc strerrorname_np

[Florian Weimer]

* Adhemerval Zanella via Libc-alpha:

> But I do agree with you that at least keeping old buggy behavior for the
> sake of 'compatibility' is not a practice that I think help improve the
> whole ecosystem as whole (although it does help close-source and/or binary
> one system where recompile is an issue).

But the old behavior is arguable not a bug.  It was always expected that
the caller would check for NULL.  The NULL behavior has already made it
into Solaris:

| The returned string is not translated according to the current locale,
| and strerrordesc_np() returns a NULL result for unknown error codes,
| rather than a generated descriptive string.

<https://docs.oracle.com/cd/E88353_01/html/E37843/strerrorname-np-3c.html>

So I think this ship has sailed.

By the way, I seem to recall that an incompatible version of these
functions (different from strerror_r) has been defined in POSIX and
implemented by one of the other systems.  But I can't find the function
name anymore. 8-(

Thanks,
Florian

Re: glibc strerrorname_np

[Adhemerval Zanella]

On 08/11/2021 16:56, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> But I do agree with you that at least keeping old buggy behavior for the
>> sake of 'compatibility' is not a practice that I think help improve the
>> whole ecosystem as whole (although it does help close-source and/or binary
>> one system where recompile is an issue).
> 
> But the old behavior is arguable not a bug.  It was always expected that
> the caller would check for NULL.  The NULL behavior has already made it
> into Solaris:
> 
> | The returned string is not translated according to the current locale,
> | and strerrordesc_np() returns a NULL result for unknown error codes,
> | rather than a generated descriptive string.
> 
> <https://docs.oracle.com/cd/E88353_01/html/E37843/strerrorname-np-3c.html>
> 
> So I think this ship has sailed.

Right, I was not aware Solaris has adopted this interface so I do agree
that it is too late now and it would require a new symbol to provide
such semantic.

> 
> By the way, I seem to recall that an incompatible version of these
> functions (different from strerror_r) has been defined in POSIX and
> implemented by one of the other systems.  But I can't find the function
> name anymore. 8-(
> 
> Thanks,
> Florian
> 

Re: glibc strerrorname_np

[Jonny Grant]

On 06/11/2021 12:51, Adhemerval Zanella wrote:
> 
> 
> On 05/11/2021 19:23, Jonny Grant wrote:
>>
>>
>> On 05/11/2021 13:01, Adhemerval Zanella wrote:
>>>
>>>
>>> On 05/11/2021 08:51, Jonny Grant wrote:
>>>>>
>>>>
>>>> Hi Adhemerval
>>>>
>>>> Thank you for your reply. Personally I understood an ABI break would be the return type, the name, or the parameters. But the proposed change is not so. Changing to return a string, should be fine.
>>>
>>> It is still an ABI break, code that checks NULL for invalid input will
>>> stop to work.
>>>
>>>>
>>>> ie, in relation to strerror() C99 and POSIX.1-2008 require the return value to be non-NULL. (my view is it is always better not to return a NULL from such string functions that could then cause a SEGV.
>>>>
>>>> strerror(1000) returns a string "Unknown error 1000"
>>>>
>>>> Better to simply align with glibc strerror() approach?
>>>>
>>>> Feels like there is still time to change it, as it is _np. Aligning with strerror(), or just "" as you had mentioned seems reasonable.
>>>
>>> I give you that it is indeed a better return code, and it is not a matter
>>> of timing, but rather I don't think it really worth the ABI break and 
>>> the required code complexity to do so. 
>>>
>>> It would require:
>>>
>>>   1. Change the strerrorname_np to return "" on invalid code.
>>
>> Please find attached the patch.
>>
>>>   2. Keep the compat symbol that returns NULL and add a compat symbol.
>>>   3. Exports a new symbol with version on 2.35 with the new semantic
>>>      and update the ailist.

Could you dircet me to the ailist file you mention please.

>>
>> May I check, why would a new symbol be needed? I'd expect it is only a change to strerrorname_np and any test code you have that presently checks for NULL return.
> 
> As I said before it is an ABI break, since users that check for invalid 
> errno against NULL will start to fail.  For such change we *do need* all 
> the trouble of adding a compat symbol with current semantic.
Many thanks for your reply. May I check, are even the _np functions set in stone once they are released? glibc strerrorname_np was released a year ago. My disto Ubuntu LTS doesn't yet have this new glibc release containing this function.

>>
>>>   4. Update the documentation and sync with man-pages. 
>>
>> The man-page update is minor, I could handle that.
>>
>>
>>>> https://man7.org/linux/man-pages/man3/strerror.3.html
>>>>
>>>> It's common for some returns to change, eg glibc 2.13 changed strerror_r() behaviour to return the actual error code, as opposed to returning -1 and setting errno.
>>>
>>> And such change did got without burden and extra complexity. Just check
>>> the multiple preprocessor checks it requires to get the right definition
>>> depending of the system support on the misc/error.c (imported from gnulib).
>>>
>>
>> Ok, I think the change I propose does not affect the definition, as the function signature is the same. Maybe I misunderstand something.
> 
> The ABI break is not only for function signature and input alignment/size,
> but also for function semantic.  Just check the fmemopen
> (fdb7d390dd0d96e4a8239c46f3aa64598b90842b), where we kept the old buggy 
> implementation since even when it is not POSIX compliant because we do
> not know if users do depend of such behavior.
> 

I was reading the fmemopen ticket you shared. May I ask - could you direct me to a "compat symbol" description? I did search and came across libc_hidden_proto and see it in use in /glibc/include/stdio.h the patch commit note says update fmemopen to the new POSIX spec.

Kind regards
Jonny

Re: glibc strerrorname_np

[Adhemerval Zanella]

On 08/11/2021 19:22, Jonny Grant wrote:
> 
> 
> On 06/11/2021 12:51, Adhemerval Zanella wrote:
>>
>>
>> On 05/11/2021 19:23, Jonny Grant wrote:
>>>
>>>
>>> On 05/11/2021 13:01, Adhemerval Zanella wrote:
>>>>
>>>>
>>>> On 05/11/2021 08:51, Jonny Grant wrote:
>>>>>>
>>>>>
>>>>> Hi Adhemerval
>>>>>
>>>>> Thank you for your reply. Personally I understood an ABI break would be the return type, the name, or the parameters. But the proposed change is not so. Changing to return a string, should be fine.
>>>>
>>>> It is still an ABI break, code that checks NULL for invalid input will
>>>> stop to work.
>>>>
>>>>>
>>>>> ie, in relation to strerror() C99 and POSIX.1-2008 require the return value to be non-NULL. (my view is it is always better not to return a NULL from such string functions that could then cause a SEGV.
>>>>>
>>>>> strerror(1000) returns a string "Unknown error 1000"
>>>>>
>>>>> Better to simply align with glibc strerror() approach?
>>>>>
>>>>> Feels like there is still time to change it, as it is _np. Aligning with strerror(), or just "" as you had mentioned seems reasonable.
>>>>
>>>> I give you that it is indeed a better return code, and it is not a matter
>>>> of timing, but rather I don't think it really worth the ABI break and 
>>>> the required code complexity to do so. 
>>>>
>>>> It would require:
>>>>
>>>>   1. Change the strerrorname_np to return "" on invalid code.
>>>
>>> Please find attached the patch.
>>>
>>>>   2. Keep the compat symbol that returns NULL and add a compat symbol.
>>>>   3. Exports a new symbol with version on 2.35 with the new semantic
>>>>      and update the ailist.
> 
> Could you dircet me to the ailist file you mention please.

I spelled it wrong, it should be 'abilist':

$ find . -iname *.abilist
./sysdeps/unix/sysv/linux/x86_64/64/libBrokenLocale.abilist
./sysdeps/unix/sysv/linux/x86_64/64/libm.abilist
./sysdeps/unix/sysv/linux/x86_64/64/libutil.abilist
[...]

You can check if the exported symbols are on par with the pre-defined
ones with:

$ make check-abi

And you can update the abilist files with:

$ make update-abi

It will need to be properly exported on the 'Versions' file.


> 
>>>
>>> May I check, why would a new symbol be needed? I'd expect it is only a change to strerrorname_np and any test code you have that presently checks for NULL return.
>>
>> As I said before it is an ABI break, since users that check for invalid 
>> errno against NULL will start to fail.  For such change we *do need* all 
>> the trouble of adding a compat symbol with current semantic.
> Many thanks for your reply. May I check, are even the _np functions set in stone once they are released? glibc strerrorname_np was released a year ago. My disto Ubuntu LTS doesn't yet have this new glibc release containing this function.

It is not a matter of time, neither glibc is tied to any distribution.  It is
a matter or point release, for instance 2.34.  Once we export such symbol on
a release, programs will be built against and depend of the symbol.

> 
>>>
>>>>   4. Update the documentation and sync with man-pages. 
>>>
>>> The man-page update is minor, I could handle that.
>>>
>>>
>>>>> https://man7.org/linux/man-pages/man3/strerror.3.html
>>>>>
>>>>> It's common for some returns to change, eg glibc 2.13 changed strerror_r() behaviour to return the actual error code, as opposed to returning -1 and setting errno.
>>>>
>>>> And such change did got without burden and extra complexity. Just check
>>>> the multiple preprocessor checks it requires to get the right definition
>>>> depending of the system support on the misc/error.c (imported from gnulib).
>>>>
>>>
>>> Ok, I think the change I propose does not affect the definition, as the function signature is the same. Maybe I misunderstand something.
>>
>> The ABI break is not only for function signature and input alignment/size,
>> but also for function semantic.  Just check the fmemopen
>> (fdb7d390dd0d96e4a8239c46f3aa64598b90842b), where we kept the old buggy 
>> implementation since even when it is not POSIX compliant because we do
>> not know if users do depend of such behavior.
>>
> 
> I was reading the fmemopen ticket you shared. May I ask - could you direct me to a "compat symbol" description? I did search and came across libc_hidden_proto and see it in use in /glibc/include/stdio.h the patch commit note says update fmemopen to the new POSIX spec.

To put simply, the 'compat symbol' is a GNU extension that creates a 
symbol tied to an specific version where a linked program built against 
an older glibc will continue to use the older version instead of the 
default one.

There is some information on how it is created internally on
'include/shlib-compat.h'.

Re: glibc strerrorname_np

[Jonny Grant]

On 09/11/2021 12:30, Adhemerval Zanella wrote:
> 
> 
> On 08/11/2021 19:22, Jonny Grant wrote:
>>
>>
>> On 06/11/2021 12:51, Adhemerval Zanella wrote:
>>>
>>>
>>> On 05/11/2021 19:23, Jonny Grant wrote:
>>>>
>>>>
>>>> On 05/11/2021 13:01, Adhemerval Zanella wrote:
>>>>>
>>>>>
>>>>> On 05/11/2021 08:51, Jonny Grant wrote:
>>>>>>>
>>>>>>
>>>>>> Hi Adhemerval
>>>>>>
>>>>>> Thank you for your reply. Personally I understood an ABI break would be the return type, the name, or the parameters. But the proposed change is not so. Changing to return a string, should be fine.
>>>>>
>>>>> It is still an ABI break, code that checks NULL for invalid input will
>>>>> stop to work.
>>>>>
>>>>>>
>>>>>> ie, in relation to strerror() C99 and POSIX.1-2008 require the return value to be non-NULL. (my view is it is always better not to return a NULL from such string functions that could then cause a SEGV.
>>>>>>
>>>>>> strerror(1000) returns a string "Unknown error 1000"
>>>>>>
>>>>>> Better to simply align with glibc strerror() approach?
>>>>>>
>>>>>> Feels like there is still time to change it, as it is _np. Aligning with strerror(), or just "" as you had mentioned seems reasonable.
>>>>>
>>>>> I give you that it is indeed a better return code, and it is not a matter
>>>>> of timing, but rather I don't think it really worth the ABI break and 
>>>>> the required code complexity to do so. 
>>>>>
>>>>> It would require:
>>>>>
>>>>>   1. Change the strerrorname_np to return "" on invalid code.
>>>>
>>>> Please find attached the patch.
>>>>
>>>>>   2. Keep the compat symbol that returns NULL and add a compat symbol.
>>>>>   3. Exports a new symbol with version on 2.35 with the new semantic
>>>>>      and update the ailist.
>>
>> Could you dircet me to the ailist file you mention please.
> 
> I spelled it wrong, it should be 'abilist':
> 
> $ find . -iname *.abilist
> ./sysdeps/unix/sysv/linux/x86_64/64/libBrokenLocale.abilist
> ./sysdeps/unix/sysv/linux/x86_64/64/libm.abilist
> ./sysdeps/unix/sysv/linux/x86_64/64/libutil.abilist
> [...]
> 
> You can check if the exported symbols are on par with the pre-defined
> ones with:
> 
> $ make check-abi
> 
> And you can update the abilist files with:
> 
> $ make update-abi
> 
> It will need to be properly exported on the 'Versions' file.
> 
> 
>>
>>>>
>>>> May I check, why would a new symbol be needed? I'd expect it is only a change to strerrorname_np and any test code you have that presently checks for NULL return.
>>>
>>> As I said before it is an ABI break, since users that check for invalid 
>>> errno against NULL will start to fail.  For such change we *do need* all 
>>> the trouble of adding a compat symbol with current semantic.
>> Many thanks for your reply. May I check, are even the _np functions set in stone once they are released? glibc strerrorname_np was released a year ago. My disto Ubuntu LTS doesn't yet have this new glibc release containing this function.
> 
> It is not a matter of time, neither glibc is tied to any distribution.  It is
> a matter or point release, for instance 2.34.  Once we export such symbol on
> a release, programs will be built against and depend of the symbol.

Ok I see.
It's also tricky as I'd prefer to not define _GNU_SOURCE

What do you think of the possiblity of adding something like strerrorname() directly without _GNU_SOURCE, and always returning a valid string, or empty string?

With a consistent return for an errnum of 0.
strerror_r(0, &b, 0); already returns "".
strerror(0) returns "Success"

So there is some inconsistency in the POSIX functions.




>>
>>>>
>>>>>   4. Update the documentation and sync with man-pages. 
>>>>
>>>> The man-page update is minor, I could handle that.
>>>>
>>>>
>>>>>> https://man7.org/linux/man-pages/man3/strerror.3.html
>>>>>>
>>>>>> It's common for some returns to change, eg glibc 2.13 changed strerror_r() behaviour to return the actual error code, as opposed to returning -1 and setting errno.
>>>>>
>>>>> And such change did got without burden and extra complexity. Just check
>>>>> the multiple preprocessor checks it requires to get the right definition
>>>>> depending of the system support on the misc/error.c (imported from gnulib).
>>>>>
>>>>
>>>> Ok, I think the change I propose does not affect the definition, as the function signature is the same. Maybe I misunderstand something.
>>>
>>> The ABI break is not only for function signature and input alignment/size,
>>> but also for function semantic.  Just check the fmemopen
>>> (fdb7d390dd0d96e4a8239c46f3aa64598b90842b), where we kept the old buggy 
>>> implementation since even when it is not POSIX compliant because we do
>>> not know if users do depend of such behavior.
>>>
>>
>> I was reading the fmemopen ticket you shared. May I ask - could you direct me to a "compat symbol" description? I did search and came across libc_hidden_proto and see it in use in /glibc/include/stdio.h the patch commit note says update fmemopen to the new POSIX spec.
> 
> To put simply, the 'compat symbol' is a GNU extension that creates a 
> symbol tied to an specific version where a linked program built against 
> an older glibc will continue to use the older version instead of the 
> default one.
> 
> There is some information on how it is created internally on
> 'include/shlib-compat.h'.
> 

I see. Thank you for explaining.

Kind regards
Jonny

Re: glibc strerrorname_np

[Adhemerval Zanella]

On 09/11/2021 20:01, Jonny Grant wrote:
> Ok I see.
> It's also tricky as I'd prefer to not define _GNU_SOURCE
> 
> What do you think of the possiblity of adding something like strerrorname() directly without _GNU_SOURCE, and always returning a valid string, or empty string?
> 
> With a consistent return for an errnum of 0.
> strerror_r(0, &b, 0); already returns "".
> strerror(0) returns "Success"
> 
> So there is some inconsistency in the POSIX functions.

The problem of exporting it without being a GNU extension (within _GNU_SOURCE
namespace) is that it might clash with a future POSIX interface.  I need to
check where exactly POSIX defines it, but if I recall correctly it defines
specific namespace rules for the POSIX defined headers.

Re: glibc strerrorname_np

[Florian Weimer]

* Jonny Grant:

> I noticed on the man page it may return NULL, which is a shame, as then it means we always need to check that before using in every printf etc :-
>
> printf("err %s\n", strerrorname_np(myerr)?strerrorname_np(myerr), "Unknown err");

We can add %#m to printf for this use case.  It would print the errno
constant if known, or a number otherwise.  To use it, errno needs to be
assigned before calling printf.  Printing multiple error codes would not
be possible directly, but such usage is quite rare.

On older glibc, it prints the (translated) string, which is better than
nothing.

Thanks,
Florian

Re: glibc strerrorname_np

[Jonny Grant]

On 08/11/2021 08:36, Florian Weimer wrote:
> * Jonny Grant:
> 
>> I noticed on the man page it may return NULL, which is a shame, as then it means we always need to check that before using in every printf etc :-
>>
>> printf("err %s\n", strerrorname_np(myerr)?strerrorname_np(myerr), "Unknown err");
> 
> We can add %#m to printf for this use case.  It would print the errno
> constant if known, or a number otherwise.  To use it, errno needs to be
> assigned before calling printf.  Printing multiple error codes would not
> be possible directly, but such usage is quite rare.

Hi Florian

I couldn't find a reference description of %#m - May I ask if you have a link? I see %m is a GNU printf extension.

Still feels risky, relying on all calling code that currently uses strerror() to also change from %s to %#m to avoid a NULL ptr SEGV.

With kind regards
Jonny

Re: glibc strerrorname_np

[Florian Weimer]

* Jonny Grant:

> On 08/11/2021 08:36, Florian Weimer wrote:
>> * Jonny Grant:
>> 
>>> I noticed on the man page it may return NULL, which is a shame, as then it means we always need to check that before using in every printf etc :-
>>>
>>> printf("err %s\n", strerrorname_np(myerr)?strerrorname_np(myerr), "Unknown err");
>> 
>> We can add %#m to printf for this use case.  It would print the errno
>> constant if known, or a number otherwise.  To use it, errno needs to be
>> assigned before calling printf.  Printing multiple error codes would not
>> be possible directly, but such usage is quite rare.
>
> Hi Florian
>
> I couldn't find a reference description of %#m - May I ask if you have
> a link? I see %m is a GNU printf extension.

Sorry, it would be a variant of %m, printing a known error constant or
the error number.  It's not implemented yet.

Thanks,
Florian

Re: glibc strerrorname_np

[Jonny Grant]

On 08/11/2021 13:42, Florian Weimer wrote:
> * Jonny Grant:
> 
>> On 08/11/2021 08:36, Florian Weimer wrote:
>>> * Jonny Grant:
>>>
>>>> I noticed on the man page it may return NULL, which is a shame, as then it means we always need to check that before using in every printf etc :-
>>>>
>>>> printf("err %s\n", strerrorname_np(myerr)?strerrorname_np(myerr), "Unknown err");
>>>
>>> We can add %#m to printf for this use case.  It would print the errno
>>> constant if known, or a number otherwise.  To use it, errno needs to be
>>> assigned before calling printf.  Printing multiple error codes would not
>>> be possible directly, but such usage is quite rare.
>>
>> Hi Florian
>>
>> I couldn't find a reference description of %#m - May I ask if you have
>> a link? I see %m is a GNU printf extension.
> 
> Sorry, it would be a variant of %m, printing a known error constant or
> the error number.  It's not implemented yet.

Ok I see. Probably I wouldn't want to rely on a printf formatting that was specific to glibc.
Kind regards, Jonny

Re: glibc strerrorname_np

[Adhemerval Zanella]

On 08/11/2021 05:36, Florian Weimer via Libc-alpha wrote:
> * Jonny Grant:
> 
>> I noticed on the man page it may return NULL, which is a shame, as then it means we always need to check that before using in every printf etc :-
>>
>> printf("err %s\n", strerrorname_np(myerr)?strerrorname_np(myerr), "Unknown err");
> 
> We can add %#m to printf for this use case.  It would print the errno
> constant if known, or a number otherwise.  To use it, errno needs to be
> assigned before calling printf.  Printing multiple error codes would not
> be possible directly, but such usage is quite rare.
> 
> On older glibc, it prints the (translated) string, which is better than
> nothing.
> 

This is not a fully replacement, but I think is a reasonable extension.