| Message ID | 20171221225453.13158-1-aurelien@aurel32.net |
|---|---|
| Headers | show |
| Series | Fix wrong assumption about errno | expand |
On 12/21/2017 02:54 PM, Aurelien Jarno wrote: > These 3 patches fixes the wrong assumption that a successful function > does not change errno. POSIX explicitly says that applications should > check errno only after failure (or if the function specification > provides additional scenarios where it has a defined value). > > Aurelien Jarno (3): > tst-realloc: do not check for errno on success [BZ #22611] > manual: clarify errno value on success [BZ #22615] > scandir: fix wrong assumption about errno [BZ #17804] > > ChangeLog | 18 ++++++++++++++++++ > dirent/scandir-tail.c | 7 +++++-- > malloc/tst-realloc.c | 4 ---- > manual/errno.texi | 12 ++++++------ > 4 files changed, 29 insertions(+), 12 deletions(-) Thank you very much for cleaning these things up. Incremental improvements like this bring me holiday cheer :-)
On Thu, Dec 21, 2017 at 2:54 PM, Aurelien Jarno <aurelien@aurel32.net> wrote: > These 3 patches fixes the wrong assumption that a successful function > does not change errno. POSIX explicitly says that applications should > check errno only after failure (or if the function specification > provides additional scenarios where it has a defined value). I like these changes in general, but there are a few specific cases where functions _are_ guaranteed not to change the value of errno on success, and application code relies on that. The one I remember off the top of my head is the strto* family, where all possible return values _could_ be the result of a successful parse, so you have to set errno to 0 beforehand and then check it afterward. Please make sure that your changes do not imply this is not the case. zw
On 12/21/2017 07:40 PM, Zack Weinberg wrote: > On Thu, Dec 21, 2017 at 2:54 PM, Aurelien Jarno <aurelien@aurel32.net> wrote: >> These 3 patches fixes the wrong assumption that a successful function >> does not change errno. POSIX explicitly says that applications should >> check errno only after failure (or if the function specification >> provides additional scenarios where it has a defined value). > > I like these changes in general, but there are a few specific cases > where functions _are_ guaranteed not to change the value of errno on > success, and application code relies on that. The one I remember off > the top of my head is the strto* family, where all possible return > values _could_ be the result of a successful parse, so you have to set > errno to 0 beforehand and then check it afterward. Please make sure > that your changes do not imply this is not the case. The CERT coding rules[1] cover the list. What you mention is called in-band error return, where the function error return cannot be disambiguated from a correct result. The manual clarification is more correct than we had before. We say that no function will set errno to zero, but may set it to non-zero when they succeed. This does not preclude what you are talking about with functions that have in-band error return.
On Thu, Dec 21, 2017 at 8:08 PM, Carlos O'Donell <carlos@redhat.com> wrote: > On 12/21/2017 07:40 PM, Zack Weinberg wrote: >> On Thu, Dec 21, 2017 at 2:54 PM, Aurelien Jarno <aurelien@aurel32.net> wrote: >>> These 3 patches fixes the wrong assumption that a successful function >>> does not change errno. POSIX explicitly says that applications should >>> check errno only after failure (or if the function specification >>> provides additional scenarios where it has a defined value). >> >> I like these changes in general, but there are a few specific cases >> where functions _are_ guaranteed not to change the value of errno on >> success, and application code relies on that. The one I remember off >> the top of my head is the strto* family, where all possible return >> values _could_ be the result of a successful parse, so you have to set >> errno to 0 beforehand and then check it afterward. Please make sure >> that your changes do not imply this is not the case. > > The CERT coding rules[1] cover the list. What you mention is called in-band > error return, where the function error return cannot be disambiguated from a > correct result. > > The manual clarification is more correct than we had before. We say that > no function will set errno to zero, but may set it to non-zero when they > succeed. This does not preclude what you are talking about with functions > that have in-band error return. Please reread what I said - it's subtler than that. The key point is that there *are* functions in the GNU C Library that are guaranteed *not* to set errno to a nonzero value unless they have actually failed, such as strto*. It's well and good for the general discussion of errno to talk about how *most* functions do not make this guarantee, but they should not make it sound like *none* of them do, and the specific documentation for the functions that do make that guarantee should say so. (N.B. neither C11 nor POSIX makes the above an explicit requirement for strto*, as I read them, but it is a de facto requirement given the documented idiom for checking for failure // p is expected to be a C-string containing a number and nothing more errno = 0; val = strtol(p, &endp, 0); if (p == endp || *endp || errno) report_error(); .) zw
On 2017-12-21 20:18, Zack Weinberg wrote: > On Thu, Dec 21, 2017 at 8:08 PM, Carlos O'Donell <carlos@redhat.com> wrote: > > On 12/21/2017 07:40 PM, Zack Weinberg wrote: > >> On Thu, Dec 21, 2017 at 2:54 PM, Aurelien Jarno <aurelien@aurel32.net> wrote: > >>> These 3 patches fixes the wrong assumption that a successful function > >>> does not change errno. POSIX explicitly says that applications should > >>> check errno only after failure (or if the function specification > >>> provides additional scenarios where it has a defined value). > >> > >> I like these changes in general, but there are a few specific cases > >> where functions _are_ guaranteed not to change the value of errno on > >> success, and application code relies on that. The one I remember off > >> the top of my head is the strto* family, where all possible return > >> values _could_ be the result of a successful parse, so you have to set > >> errno to 0 beforehand and then check it afterward. Please make sure > >> that your changes do not imply this is not the case. > > > > The CERT coding rules[1] cover the list. What you mention is called in-band > > error return, where the function error return cannot be disambiguated from a > > correct result. > > > > The manual clarification is more correct than we had before. We say that > > no function will set errno to zero, but may set it to non-zero when they > > succeed. This does not preclude what you are talking about with functions > > that have in-band error return. > > Please reread what I said - it's subtler than that. The key point is > that there *are* functions in the GNU C Library that are guaranteed > *not* to set errno to a nonzero value unless they have actually > failed, such as strto*. It's well and good for the general discussion > of errno to talk about how *most* functions do not make this > guarantee, but they should not make it sound like *none* of them do, > and the specific documentation for the functions that do make that > guarantee should say so. This means that the remaining part of the sectiont is also wrong: "..., and you should not use @code{errno} to determine @emph{whether} a call failed." I guess the correct way to fix that would be to define in-band and out-of-band error reporting like in the CERT coding rules. But that's a lot more job than just clarifying the existing section. In the meantime, what about the following text: | The initial value of @code{errno} at program startup is zero. Many | library functions are guaranteed to set it to certain non-zero values | when they encounter certain kinds of errors. These error conditions are | listed for each function. Some of these functions are guaranteed to set | @code{errno} only in case of failure, while some other might set it to a | non-zero value even after a successful call. In any case these functions | never set @code{errno} to zero. The proper way to check for error is | documented for each function. Aurelien
On Fri, Dec 22, 2017 at 5:55 AM, Aurelien Jarno <aurelien@aurel32.net> wrote: > On 2017-12-21 20:18, Zack Weinberg wrote: >> Please reread what I said - it's subtler than that. The key point is >> that there *are* functions in the GNU C Library that are guaranteed >> *not* to set errno to a nonzero value unless they have actually >> failed, such as strto*. It's well and good for the general discussion >> of errno to talk about how *most* functions do not make this >> guarantee, but they should not make it sound like *none* of them do, >> and the specific documentation for the functions that do make that >> guarantee should say so. > > This means that the remaining part of the sectiont is also wrong: > > "..., and you should not use @code{errno} to determine @emph{whether} a > call failed." > > I guess the correct way to fix that would be to define in-band and > out-of-band error reporting like in the CERT coding rules. But that's a > lot more job than just clarifying the existing section. In the meantime, > what about the following text: > > | The initial value of @code{errno} at program startup is zero. Many > | library functions are guaranteed to set it to certain non-zero values > | when they encounter certain kinds of errors. These error conditions are > | listed for each function. Some of these functions are guaranteed to set > | @code{errno} only in case of failure, while some other might set it to a > | non-zero value even after a successful call. In any case these functions > | never set @code{errno} to zero. The proper way to check for error is > | documented for each function. Well, Carlos is right that it is _usually_ wrong to check errno to decide if something failed. Maybe something in the middle: | The initial value of @code{errno} at program startup is zero. In many | cases, when a library function encounters an error, it will set | @code{errno} to a non-zero value to indicate what specific error | condition occurred. The documentation for each function lists the | error conditions that are possible for that function. Not all library | functions use this mechanism; some return an error code directly, | instead. | | @strong{Warning:} Many library functions may set @code{errno} to some | meaningless non-zero value even if they did not encounter any errors, | and even if they return error codes directly. Therefore, it is | usually incorrect to check @emph{whether} an error occurred by | inspecting the value of @code{errno}. The proper way to check for | error is documented for each function.
On 12/22/2017 08:39 AM, Zack Weinberg wrote: > Well, Carlos is right that it is _usually_ wrong to check errno to > decide if something failed. Maybe something in the middle: > > | The initial value of @code{errno} at program startup is zero. In many > | cases, when a library function encounters an error, it will set > | @code{errno} to a non-zero value to indicate what specific error > | condition occurred. The documentation for each function lists the > | error conditions that are possible for that function. Not all library > | functions use this mechanism; some return an error code directly, > | instead. > | > | @strong{Warning:} Many library functions may set @code{errno} to some > | meaningless non-zero value even if they did not encounter any errors, > | and even if they return error codes directly. Therefore, it is > | usually incorrect to check @emph{whether} an error occurred by > | inspecting the value of @code{errno}. The proper way to check for > | error is documented for each function. Zack, This language is better than what we have, thank you for that. Aurelien, Are you able to merge this in to the manual?
On 2017-12-22 08:57, Carlos O'Donell wrote: > On 12/22/2017 08:39 AM, Zack Weinberg wrote: > > Well, Carlos is right that it is _usually_ wrong to check errno to > > decide if something failed. Maybe something in the middle: > > > > | The initial value of @code{errno} at program startup is zero. In many > > | cases, when a library function encounters an error, it will set > > | @code{errno} to a non-zero value to indicate what specific error > > | condition occurred. The documentation for each function lists the > > | error conditions that are possible for that function. Not all library > > | functions use this mechanism; some return an error code directly, > > | instead. > > | > > | @strong{Warning:} Many library functions may set @code{errno} to some > > | meaningless non-zero value even if they did not encounter any errors, > > | and even if they return error codes directly. Therefore, it is > > | usually incorrect to check @emph{whether} an error occurred by > > | inspecting the value of @code{errno}. The proper way to check for > > | error is documented for each function. > > Zack, This language is better than what we have, thank you for that. Indeed, thanks Zack. > Aurelien, Are you able to merge this in to the manual? I'll send a v2 with the above changes to the manual and with the changes you suggested to the third patch. Thanks, Aurelien