Re: [PATCH] A Problem in compat_level_from_string()
Okay. After I identified the code which was causing the problem, my initial fix was to clear errno before each call to strtol(), and that resolved the problem. I then went in and looked at the code in more detail, as well as read the spec for strtol(). My change will work also, since strtol() returns the original string pointer through endptr only when it cannot do any conversion. I am not concerned as long as the code functions properly. David On Tue, Jun 22, 2021 at 11:13 AM Viktor Dukhovni wrote: > > On Tue, Jun 22, 2021 at 10:49:49AM -0700, David Bohman wrote: > > > You cannot assume that the value returned to 'endptr' is greater than > > 'str' on a valid result. It could be a different string entirely, with > > a lesser pointer value. That is up to the implementation. > > Postfix does not pass arguments to strtol() that violate pointer > aliasing. We can and do correctly assume that strtol() behaves > correctly, and returns a value of 'endptr' that is >= the source > string. > > -- > Viktor.
Re: [PATCH] A Problem in compat_level_from_string()
On Tue, Jun 22, 2021 at 10:49:49AM -0700, David Bohman wrote: > You cannot assume that the value returned to 'endptr' is greater than > 'str' on a valid result. It could be a different string entirely, with > a lesser pointer value. That is up to the implementation. Postfix does not pass arguments to strtol() that violate pointer aliasing. We can and do correctly assume that strtol() behaves correctly, and returns a value of 'endptr' that is >= the source string. -- Viktor.
Re: [PATCH] A Problem in compat_level_from_string()
Answers below. On Tue, Jun 22, 2021 at 9:51 AM Wietse Venema wrote: > > David Bohman: > > This is apparently a new routine in version 3.6. > > > > I upgraded from version 3.5.9 directly to 3.6.1 and ran into an issue. > > Postfix failed to start up without any diagnostic output. It took me a bit > > to narrow down the failure, but I discovered that this routine was failing > > on valid input. > > What was the valid input? The valid input was originally "2". I later tried "3.6" (which I am running with now). > > > Use of the library routine strtol() is problematic due to the lack of a > > direct failure indication. You cannot check errno for a value unless you > > zero it before the invocation. > > Good point. I notice that most Postfix code will reset errno to > zero before calling strtol() and the like, but it is missing in the > compatibility level parser (and some other place). I'll add that. > > > But, I am thinking that we don't really need to check for an overflow or > > underflow error from strtol(), since GOOD_MAJOR() and friends already do > > range checking. My solution is to simply remove the errno checks. > > Range checks make no sense if strtol() failed. So deleting that > test is wrong. In the case of a range error from strtol(), the returned value is clamped to either LONG_MIN or LONG_MAX, so you can do a range check on that. > > > One other modification I made concerns the comparisons "start < remainder", > > which I changed to "start != remainder". There is no guarantee concerning > > the location of the buffer returned via "endptr". Its relation to the > > original string is undefined, unless no conversion was done, in which case > > it returns the original string. > > strtol returns the position of the first 'invalid' character, > or the start of the input when no conversion was done. Yes, but due to the use of "restrict" on 'str' and 'endptr', you cannot assume that the pointer returned to 'endptr' is greater than 'str'. The spec only says that: "If the subject sequence is empty or does not have the expected form, no conversion is performed; the value of str is stored in the object pointed to by endptr, provided that endptr is not a null pointer." You cannot assume that the value returned to 'endptr' is greater than 'str' on a valid result. It could be a different string entirely, with a lesser pointer value. That is up to the implementation. > > Wietse David
Re: [PATCH] A Problem in compat_level_from_string()
David Bohman: > This is apparently a new routine in version 3.6. > > I upgraded from version 3.5.9 directly to 3.6.1 and ran into an issue. > Postfix failed to start up without any diagnostic output. It took me a bit > to narrow down the failure, but I discovered that this routine was failing > on valid input. What was the valid input? > Use of the library routine strtol() is problematic due to the lack of a > direct failure indication. You cannot check errno for a value unless you > zero it before the invocation. Good point. I notice that most Postfix code will reset errno to zero before calling strtol() and the like, but it is missing in the compatibility level parser (and some other place). I'll add that. > But, I am thinking that we don't really need to check for an overflow or > underflow error from strtol(), since GOOD_MAJOR() and friends already do > range checking. My solution is to simply remove the errno checks. Range checks make no sense if strtol() failed. So deleting that test is wrong. > One other modification I made concerns the comparisons "start < remainder", > which I changed to "start != remainder". There is no guarantee concerning > the location of the buffer returned via "endptr". Its relation to the > original string is undefined, unless no conversion was done, in which case > it returns the original string. strtol returns the position of the first 'invalid' character, or the start of the input when no conversion was done. Wietse
