From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from fhigh3-smtp.messagingengine.com (fhigh3-smtp.messagingengine.com [103.168.172.154]) by sourceware.org (Postfix) with ESMTPS id 4A3A13858D28 for ; Fri, 22 Mar 2024 14:24:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 4A3A13858D28 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=owlfolio.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=owlfolio.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 4A3A13858D28 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=103.168.172.154 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1711117499; cv=none; b=WAsmbxoFBRwC4ZBY955C/NRb84xxfEF0+gCyBNhVrjd7gRrJYATOi9aCM/RqTfMvnHmbP8E3IGtuzy/WHgIxERa219F2VsLFN2pUVjEga1Vx/7Fm43g6EVhbht0O+cL5/gAffPnzZElLDklQgA2M7zdItwRabi4OCT1nZhep7RY= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1711117499; c=relaxed/simple; bh=zYaCNPZsHDoV4gxx6Ft2Xv1D3czOh0RTgxExGzLT9OM=; h=DKIM-Signature:DKIM-Signature:MIME-Version:Message-Id:Date:From: To:Subject; b=td0TkIN0qUXJO6Omf8TFa9+LlAWBAWTpPWzVDwWpVpyqJs+kY/lSjwnqKkAwkiGEx9hBZUs1Vk4ICCwEAbjzpC5woyzBtEXDNUvCK7wQ1KozPgE5+Ham1DqnZkrD/QYhFTabqZ0XXArjlFlTuo2faWjaQyrPf0LCkFLkZmHIizk= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailfhigh.nyi.internal (Postfix) with ESMTP id D884F114016C; Fri, 22 Mar 2024 10:24:56 -0400 (EDT) Received: from imap45 ([10.202.2.95]) by compute5.internal (MEProxy); Fri, 22 Mar 2024 10:24:56 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=owlfolio.org; h= cc:cc:content-type:content-type:date:date:from:from:in-reply-to :in-reply-to:message-id:mime-version:references:reply-to:subject :subject:to:to; s=fm1; t=1711117496; x=1711203896; bh=cM4BBDbKIH aAci1697iRRzDdmOyYck6T6LXM3URc9ns=; b=fjQeunON9iAkbQVfYBVzdyGGtb tE4inExvc3XXw50ZWrVpABL6WgzcJ+nDdDu36iXhArn/ZL57eF0laEpvMn+sSurw 8s6VfU8q+nibGW3ngxzpSGjq4aMDihdE+5hpp56UJma7KhuwQu440my7hwDNwVE1 MEmJdJzshMUPe+N9Wt3W9OZwrMqz5DcrOcTVTw6J6NKE2mr1tHlrOG7gQlMrklkt MB/RHvbSnFv0AZ/ftBk+BqLXV9xIcFd8yY4q7HE3WoBc+w5s8mFMpLyrY04io+IR nTBL8tsPLxmmHHOH2IavIJhq6ceyDLDibNU/Mxogpz53gcS1wtg700UCMFlw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:content-type:date:date :feedback-id:feedback-id:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:subject:subject:to :to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm2; t=1711117496; x=1711203896; bh=cM4BBDbKIHaAci1697iRRzDdmOyY ck6T6LXM3URc9ns=; b=ByDTmKdLAhHwM4K4sXoC5A/2IEKxtvq9nPtpR6QuNNPk bsU8h8FNh3Oi2qqZt27+5yhvV/yrP135d/lGbfZ9DjiGHIntpDq4IkHIcEECGiIQ Qjm+hd0NzvYQEd029hBsVzU5kMosqnlchvq1ASDGSoDsVrijSLOVW4K1m0qop2G8 1Xg7g2rea6kImCYwc8dsi0OpKuh/o4bEkQg/N06ZH6Zpbn68JVeoEsbIclgHiByS y0F1+uRMnMVVQYv0z1UwUTCwbSW4ggb54IvD0CrRWi03LaT/GpghjREuAX/bQBVW +MH+Vkh599bPr0FpnSytzQ6BcYT1tBUvu/t2UiZCSA== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvledruddtvdcutefuodetggdotefrodftvfcurf hrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecuuegr ihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenucfjug hrpefofgggkfgjfhffhffvvefutgesthdtredtreertdenucfhrhhomhepfdgkrggtkhcu hggvihhnsggvrhhgfdcuoeiirggtkhesohiflhhfohhlihhordhorhhgqeenucggtffrrg htthgvrhhnpeegheehveevfeffteeiheegvdffvdevgfehleelgfefuedvlefhieetfefg gffgudenucffohhmrghinhepohhpvghnghhrohhuphdrohhrghenucevlhhushhtvghruf hiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpeiirggtkhesohiflhhfohhlihho rdhorhhg X-ME-Proxy: Feedback-ID: i876146a2:Fastmail Received: by mailuser.nyi.internal (Postfix, from userid 501) id 814C7272007C; Fri, 22 Mar 2024 10:24:56 -0400 (EDT) X-Mailer: MessagingEngine.com Webmail Interface User-Agent: Cyrus-JMAP/3.11.0-alpha0-332-gdeb4194079-fm-20240319.002-gdeb41940 MIME-Version: 1.0 Message-Id: <3f50bd26-9199-4174-9fde-8f890651f48e@app.fastmail.com> In-Reply-To: References: <87zfuxcdjd.fsf@oldenburg.str.redhat.com> Date: Fri, 22 Mar 2024 10:24:35 -0400 From: "Zack Weinberg" To: "Job Snijders" , "Andreas Schwab" Cc: "Florian Weimer" , "GNU libc development" Subject: Re: [PATCH] resolv: add IPv6 support to inet_net_pton() Content-Type: text/plain X-Spam-Status: No, score=-2.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,JMQ_SPF_NEUTRAL,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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