Re: [PATCH] resolv: add IPv6 support to inet_net_pton()

["Zack Weinberg" <zack@owlfolio.org>]

On Fri, Mar 22, 2024, at 12:16 AM, Job Snijders wrote:
> On Tue, Mar 19, 2024 at 10:50:14AM +0100, Andreas Schwab wrote:
>> It's still unnecessary code that makes it harder to understand, which is
>> bad.

This seems to be the crux of the disagreement - Andreas says this code
is longer than necessary and that makes it harder to understand, Job
says the extra code makes it more idiomatic and therefore *easier* to
understand. Yes?

>     "If the correct value is outside the range of representable values,
>      LONG_MAX or LONG_MIN is returned (according to the sign of the value),
>      and errno is set to [ERANGE]."
>      https://pubs.opengroup.org/onlinepubs/7908799/xsh/strtol.html
>
> The above to me means that it is not enough to just check whether errno
> is set to ERANGE, but one also has to check whether LONG_MAX or LONG_MIN
> were returned. It's an 'AND' operation.

Not quite. POSIX specs are written as directives to the implementor.
A better way to read this is: If the correct values is outside the
representable range, strtol does two things: it sets errno to ERANGE,
and it returns the representable value farthest away from zero,
with the appropriate sign.

Note also this sentence

    "Because 0, LONG_MIN and LONG_MAX are returned on error and are
    also valid returns on success, an application wishing to check
    for error situations should set errno to 0, then call strtol(),
    then check errno."

This implicitly makes a guarantee that strtol *will not* set errno
to a nonzero value unless one of the specified error conditions
occurs.  (Most C library functions are permitted to set errno nonzero
even if they succeed.  No C library function is ever allowed to set
errno to zero.)  That means it's safe to look at errno *before*
looking at the return value, as the code under discussion does.

> Then, if errno is NOT set to ERANGE, then we have to additionally
> check whether the value is within contextual bounds.

Here I agree with Andreas.  If there are contextual bounds that are
a strict subset of the representable range (i.e. neither LONG_MAX
nor LONG_MIN is contextually valid) then it *doesn't matter* whether
LONG_MAX or LONG_MIN was actually the input value or whether it was
produced as a result of overflow; it's contextually invalid either
way, so the contextual bounds check subsumes the overflow check.

> So I don't think it is correct to skimp on checks here, to me
> idiomatic checks do not equate 'bad'.

Let's recap - the code under discussion is

+	save_errno = errno;
...
+	__set_errno (0);
+	lbits = strtol(sep, &ep, 10);
+	if (sep[0] == '\0' || *ep != '\0') {
+		__set_errno (ENOENT);
+		return (-1);
+	}
+	if ((errno == ERANGE && (lbits == LONG_MAX || lbits == LONG_MIN))
+	    || (lbits > 128 || lbits < 0)) {
+		__set_errno (EMSGSIZE);
+		return (-1);
+	}
...
+       __set_errno (save_errno);

Tangentially, "sep[0] == '\0'" is not the conventional way to check
for strtol not having advanced ep at all.  It *works*, but only
because "*ep == '\0'" catches all the cases it misses, and I didn't
see that until *after* I started writing this paragraph, which
originally would have claimed that this was an outright bug.  Please
change that to "ep == sep", even if you make no other changes.

Anyway, I would have written this as

    lbits = strtol(sep, &ep, 10);
    if (ep == sep || *ep != '\0') {
        __set_errno(ENOENT);
        return -1;
    }
    if (lbits < 0 || lbits > 128) {
        __set_errno(EMSGSIZE);
        return -1;
    }

and I do think this is an improvement over what you had, largely
because it eliminates the need to save and restore the original value
of errno.  inet_pton *is* allowed to set errno nonzero despite having
succeeded, the save/restore was only necessary because of internally
setting it to *zero*, and if we're not going to look at the errno
return from strtol then we don't need that.

It *does* mean future readers have to think for a moment to confirm
that one of the three cases where strtol might set errno is impossible
(invalid base, 10 is statically valid) and the other two are covered
by the conditions below (no characters consumed -> ep == sep, overflow
-> outside the range [0, 128]).  But in this case all the numbers
involved are small enough that the extra cognitive load is reasonable.
If it was "lbits > 4294967242" then I would probably include a comment
to the effect that 4294967242 is less than the minimum value of
LONG_MAX and therefore the input overflow check is subsumed.

zw