From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-12.3 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 12800C47076 for ; Fri, 21 May 2021 14:42:17 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 5F099613E6 for ; Fri, 21 May 2021 14:42:16 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5F099613E6 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmx.de Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 9A4D482E52; Fri, 21 May 2021 16:42:14 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=fail (p=none dis=none) header.from=gmx.de Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; secure) header.d=gmx.net header.i=@gmx.net header.b="R2qcGMui"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 1C1D382E52; Fri, 21 May 2021 16:42:13 +0200 (CEST) Received: from mout.gmx.net (mout.gmx.net [212.227.17.21]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id E259482E52 for ; Fri, 21 May 2021 16:42:09 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=gmx.de Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=xypron.glpk@gmx.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gmx.net; s=badeba3b8450; t=1621608126; bh=evlrAuLPH6cWNTRdt5+Fgxp4jgPvk1wLbUSKa7d8+M8=; h=X-UI-Sender-Class:Subject:To:Cc:References:From:Date:In-Reply-To; b=R2qcGMui+MwjtZHhhaUSLuZ5dVjrj564tfnW/5QOs5k2OKZSa6ypzv5mvbA2P1M+M SCmTOy3sNZ1qrvNwDxeDii8FH1AHkwSssd0ass/PjQvWIh2uC6We+8BPSlWjPUdPKU 6D+XooTjIMGlNmDVxOuF8xhDGT84qKofNBzg2bpc= X-UI-Sender-Class: 01bb95c1-4bf8-414a-932a-4f6e2808ef9c Received: from [192.168.123.70] ([62.143.247.63]) by mail.gmx.net (mrgmx105 [212.227.17.168]) with ESMTPSA (Nemesis) id 1MHXBp-1lfTln18wD-00DWHe; Fri, 21 May 2021 16:42:06 +0200 Subject: Re: [PATCH 1/5] lib/vsprintf.c: make sure vsnprintf() never returns a negative value To: Tom Rini Cc: Rasmus Villemoes , U-Boot Mailing List , Simon Glass References: <20210520100528.322846-1-rasmus.villemoes@prevas.dk> <20210520100528.322846-2-rasmus.villemoes@prevas.dk> <6d1761b1-5129-d5a1-24ba-a27d15c42198@gmx.de> <20210521142726.GH17669@bill-the-cat> From: Heinrich Schuchardt Message-ID: Date: Fri, 21 May 2021 16:42:04 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.0 MIME-Version: 1.0 In-Reply-To: <20210521142726.GH17669@bill-the-cat> Content-Type: text/plain; charset=windows-1252 Content-Language: en-US Content-Transfer-Encoding: quoted-printable X-Provags-ID: V03:K1:CZ2tZBgDCW9FE5v/vfU0tsXDh2XNfKLaUkhWRoBNCxZ/icQZtdW 2oSShxx1ziU1T1zfqmzKYwlbbBDSjKaje4ltv8uF8lXtEbHHHW8DGTIWNGsaKoR0ayV2ciS 9eZPnIlNU5mhvJezb5lmgdb4c3Z12Ul0JhlflCj3CFHgtnAtVhD3ViUtaXhbfniCfdF4+do EEr50y3Yl0cZqmpHP47sA== X-UI-Out-Filterresults: notjunk:1;V03:K0:GDoeBJSrgiY=:45VX2HvEtRoCp0uk0Ap2oq XyiU9m9PSnfy0Yry4H8ThHAXfc0KYoVXHcu8KYF+eYup2jqbgmBp2LHgyDgU2Gm426Z3ZRF7R znM1d79Pz+v/nKqEC48bbnZJ74YX7SdNC+YcspxfFh827r5ft1CD+urk68httyC5U4jmEa18p a1blJjZm4rZPhBpKj8p2ZTEl0rIG4872Ybttdv0AyoKCFr27XM4JODMAFnqr9HpCAUerxpxcQ frj32ynbBI/wFrE5JpWh1/d25EoIAbo04fjf2GsLy7orrVJaGumIY5WhT541WufTWGTfgmip4 Pv3s+ab7KGgLSHVED3dO+5D78Q4H4JOD2Gw3KxrSHeZJ8i+1skjQkAL1hoagDmedWV4rDc85Q dn6NS/ewAMFIqqJFfYVay2WEkJHTPMpdxxG9Vc8h7ncuDMBcAgkHstLrFkyzBAn4g4NzbrU5c NLd25+hQ/QY5Rcf3db1NSqPjvCxXhkwgX/DO1WVdv8O5tek5P9ZpR54H4rfDEwQ6OnYz9vZDD eiCl3JgWMlFfsgDJBZAeMGJ5X+ud6WidQpcesKMjwjB2bt26jH8NQPu3sPVBrSAlf8DVnW6jR 0VdJIpuAwbMF+2CccDf3ZVyCVbuV9HI/3nf/4yKBcIOhDyE5lMEs4H/iUbBNZZb8eoSnop2Ly 2NiV5YLlwSZ9c3ALlklj7z5TYqof0YVWkU8AQP/gg8fX7ZsKYPR4rC+q2BFG83VQP5naoBqLR VrYH5jChcKGmN2NqVqlwxSAJm+Y36/Etj4XG1CJgmJibRCC8u810SBcq02vAAxBbxLW2+n4Ro yklea9AFLGznLYh+Byq0MF+aGT/yJg4hXE+vUh+1AzpLPChDLRKcBvmHPq7oMRpBVB1Yp+V+R e24UO0MQYQjj6cZTqp/VeGx1575Giv+HuIkReq0YEnyesGHBitZ2NrhIqlU5rOJzWIWp0ISdE zY1ylH1m3HN7QZ0I1oPvjA5sAUbjDRTLa9Lj/gy8Nt6Y6HIHbJ1NL7d0VXmg3XMuqauDzcwgo FW1oqta96FddwK+qrsvSMzkXp5ANkCAongWPN3X0dud2mxsi0hj634L1ayF1ItChUEgIxSbLi iPTITSsloJWhh1YZGpH2IvKMeokoxd4TaQPAFkc1ds0eqPGCX4AbmAZ6ju5tO4U6H0Nrpu9iP YFf+X8jpTmNM5Dvl96UjHZ7h/nXpZnQTg0eeemjtb5U/9+7C1RLPoOR4zlzcI1bIny06s= X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.102.4 at phobos.denx.de X-Virus-Status: Clean On 21.05.21 16:27, Tom Rini wrote: > On Fri, May 21, 2021 at 04:15:39PM +0200, Heinrich Schuchardt wrote: >> On 21.05.21 14:53, Rasmus Villemoes wrote: >>> On 20/05/2021 19.51, Simon Glass wrote: >>>> Hi Rasmus, >>>> >>>> On Thu, 20 May 2021 at 04:05, Rasmus Villemoes >>>> wrote: >>>>> >>>>> Most callers (or callers of callers, etc.) of vsnprintf() are not >>>>> prepared for it to return a negative value. >>>>> >>>>> The only case where that can currently happen is %pD, and it's IMO >>>>> more user-friendly to produce some output that clearly shows that so= me >>>>> "impossible" thing happened instead of having the message completely >>>>> ignored - or mishandled as for example log.c would currently do. >>>>> >>>>> Signed-off-by: Rasmus Villemoes >>>>> --- >>>>> lib/vsprintf.c | 10 +--------- >>>>> 1 file changed, 1 insertion(+), 9 deletions(-) >>>> >>>> I think that is debatable. If we want the calling code to be fixed, >>>> then it needs to get an error code back. Otherwise the error will be >>>> apparent to the user but (perhaps) not ever debugged. >>> >>> But it is not the calling code that is at fault for the vsnprintf() >>> implementation (1) being able to fail and (2) actually encountering an >>> ENOMEM situation. There's _nothing_ the calling code can do about that= . >> >> include/vsnprintf.h states: >> >> "This function follows C99 vsnprintf, but has some extensions:". >> >> The C99 spec says: >> >> "The vsnprintf function returns the number of characters that would hav= e >> been written had n been sufficiently large, not counting the >> terminating null character, or a negative value if an encoding error >> occurred." >> >> It is obvious that the calling code needs to be fixed if it cannot >> handle negative return values. >> >> So NAK to the patch. >> >> Best regards >> >> Heinrich >> >>> >>> The calling code can be said to be responsible for not passing NULL >>> pointers, but that case is actually handled gracefully in various plac= es >>> in the printf code (both for %pD, but also plain %s). >>> >>>> The definition of printf() allows for the possibility of a negative >>>> return value. >>> >>> First, please distinguish printf() from vsnprintf(). The former (in th= e >>> normal userspace version) obviously can fail for the obvious EIO, ENOS= PC >>> reasons. The latter is indeed allowed to fail per the posix spec, but >>> from a QoI perspective, I'd say it's much better to have a guarantee >>> _for our particular implementation_ that it does not fail (meaning: >>> returns a negative result). There's simply too many direct and indirec= t >>> users of vsnprintf() that assume the result is non-negative; if we do >>> not provide that guarantee, the alternative is to play a whack-a-mole >>> game and add tons of error-checking code (adding bloat to the image), >>> with almost never any good way to handle it. >>> >>> Take that log_info(" ... %pD") as an example. Suppose we "fix" log.c s= o >>> that it ignores the message if vsnprintf (or vscnprintf, whatever) >>> returns a negative result, just as print() currently does [which is th= e >>> other thing that log_info could end up being handled by]. That means >>> nothing gets printed on the console, and nobody gets told about the >>> ENOMEM. In contrast, with this patch, we get >>> >>> Booting <%pD:ENOMEM> >>> >>> printed on the console, so at least _some_ part of the message gets ou= t, >>> and it's apparent that something odd happened. Of course, all of that = is >>> in the entirely unlikely sitation where the (efi) allocation would >>> actually fail. >>> >>> If we don't want that <%pD:ENOMEM> thing, I'd still argue that we shou= ld >>> ensure vsnprintf returns non-negative; e.g. by changing the "return >>> PTR_ERR()" to a "goto out", i.e. simply stop the processing of the >>> format string at the %pD which failed, but still go through the epilog= ue >>> that ensures the resulting string becomes nul-terminated (another >>> reasonable assumption made by tons of callers), and return how much go= t >>> printed till then. > > So, how can we fix the callers without the above noted problems? > The assumption that vsnprintf() is used to print to the console and that writing some arbitrary string to the buffer is allowable is utterly wrong. vsnprintf_internal() is used to implement snprintf(). snprintf() is used in numerous places where it will not lead to console output. Trying to solve one problem this patch creates a bunch of new ones. Best regards Heinrich