[PATCH] [tldap] check for successful string conversion

[swen]

On Thu, 2019-01-10 at 13:52 +0100, Volker Lendecke via samba-technical
wrote:
> On Thu, Jan 10, 2019 at 01:04:13PM +0100, swen wrote:
> > I read it that "both" will be done in case of an error but
> > regardless
> > of how often I read the documentation I do not see the possibility
> > where errno is set but there is no error.
> 
> The spec lists two conditions about errors:
> 
> 1.: ret==0 and errno==EINVAL
> 2.: ret==ULLONG_MAX and errno==ERANGE
> 
> Those are precise error conditions with deliberately worded "and"s.
> Any other assumption about errors is not covered by the spec. Your
> patch just checks errno, but this is not covered by what the spec
> says.  "errno!=0" is not one of the conditions listed in the spec,
> and
> thus I NACK your patch as it stands.
> 
> You might reasonably argue that strtoull is a broken API, because the
> first case is optional, and thus EINVAL can not be relied upon. You
> are completely right there. But then you should come up with a better
> one, and I would propose to look at OpenBSD's strtonum or possibly
> sscanf with SCNu64, which might have better defined error handling.
> 
Volker

according to the POSIX.1-2008 and all man pages I've read so far,
there is absolutely no way how errno could be non-zero without having
an error or errno being zero but still having an error.
The combined condition you're referring to is just not part of a
Standard.

###### POSIX.1-2008 ######
ERRORS
These functions shall fail if:

[EINVAL]
The value of base is not supported. 

[ERANGE]
The value to be returned is not representable.

These functions may fail if:

[EINVAL]
No conversion could be performed. 
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

In addition the man page states
vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
Since  strtoul() can legitimately return 0 or ULONG_MAX (ULLONG_MAX for
strtoull()) on both success and failure, the calling program should set
errno to 0 before the call, and then determine if an error
occurred  by  checking  whether  errno  has  a nonzero value after the
call.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

So please reconsider you vote.

In addition to the issue discussed above, there's one case which is not
looked at yet. If the input-string represents a negative number, it is
silently accepted by the conversion functions.
This leads to a most likely unwanted result.

Since the conversion function is used quiet often in SAMBA I'd like to
go with an update in the lib/replace area as suggested.

Any comments ?

Cheers Swen
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20190110/9d55248c/signature.sig>