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=-15.3 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,MSGID_FROM_MTA_HEADER,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 436ECC47078 for ; Fri, 21 May 2021 14:48:50 +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 3967860720 for ; Fri, 21 May 2021 14:48:49 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3967860720 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=seco.com 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 681F282E1E; Fri, 21 May 2021 16:48:47 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=fail (p=none dis=none) header.from=seco.com 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; unprotected) header.d=secospa.onmicrosoft.com header.i=@secospa.onmicrosoft.com header.b="cZefJEc4"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 5DB8082E54; Fri, 21 May 2021 16:48:45 +0200 (CEST) Received: from EUR04-VI1-obe.outbound.protection.outlook.com (mail-vi1eur04on060f.outbound.protection.outlook.com [IPv6:2a01:111:f400:fe0e::60f]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 3EC2182E1E for ; Fri, 21 May 2021 16:48:42 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=seco.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=sean.anderson@seco.com ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=HKkPn0vP6c3pmKLMsdyorswbwYz/mPvMtyN7NrX9+bxABqVBVtBjBmKbP2+ldUMR9oyb8+jsxI9kcTR3CTA5y3LLT8tcBiv3NfBnepnTFoPIvYGfJMSEqO0grXJwUoTuUUWxQoPByDkewypfd1eeH5LZZYWzrKS8dRUABBCpS3qdgrfSEUZhU9lCoFDdzhsgLkp0IznYmjFPIrAion2PAK6JGtWZciRfuyxKPfdxTR8xiCGAeUxSeUosBEa6Bo0S9R1pn9finX3uxVvm29aF6/cKhIYk6yG3fZnlRsnup2145HK3LU8UleCaMAJxI1heB7pxg5Q+WYl61582W3cTpw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=OLH6FO9gh9c+dUUN3oZwf2UXFSLseZ5j/5USYn/zJFw=; b=NBPgdrHnh2j2L9GDzDWWYzA7+x4qnPiDHa+UpRKGFHQdY8bI/qh94gQDExrX/gLVLu2MsLHhZ8ZMW7qOUbupSuWvzuPWY9xTPCLIVyyHZ96fmqJeq9gpwIkCAbtXznsBo1tWTxjHUYYIxJzAXdcCQkN5g+xwoGRUMMoLGmk4UXdY1FA6Uu8vY0I2WtLBbs+/04jXMQ9GT6Q1/67Eu1s5FPAGHTyWGl0PfBWh1oMl1TUnyDoitWpADU4MwzRnfxny9JkSk8e68F99UFPz+QNBrNsilFYlf0n/krrS2p6h7LXVR1i79K3Ur7d7NgwERURwX38R4DOaeUe7D/kkhQflIw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=seco.com; dmarc=pass action=none header.from=seco.com; dkim=pass header.d=seco.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=secospa.onmicrosoft.com; s=selector2-secospa-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=OLH6FO9gh9c+dUUN3oZwf2UXFSLseZ5j/5USYn/zJFw=; b=cZefJEc4sTIgA2mANmU6x5u13WNPHb30sIR/Nd9iW3DfeADo2Rz36EMbn5Swd0n8mWolBYF3+cgc57/sjE4rzbgkVKwTAPniiuhcaTP/MpuxEBlKqZ35WSEuZFoQW1aJUTGQqxKs0utgd91/YjPCfk3l1XZZpWt38a8sbZnC+Qk= Authentication-Results: chromium.org; dkim=none (message not signed) header.d=none;chromium.org; dmarc=none action=none header.from=seco.com; Received: from DB7PR03MB4523.eurprd03.prod.outlook.com (2603:10a6:10:19::27) by DBBPR03MB7146.eurprd03.prod.outlook.com (2603:10a6:10:209::22) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4150.25; Fri, 21 May 2021 14:48:33 +0000 Received: from DB7PR03MB4523.eurprd03.prod.outlook.com ([fe80::40d5:3554:c709:6b1b]) by DB7PR03MB4523.eurprd03.prod.outlook.com ([fe80::40d5:3554:c709:6b1b%5]) with mapi id 15.20.4150.023; Fri, 21 May 2021 14:48:33 +0000 Subject: Re: [PATCH 1/5] lib/vsprintf.c: make sure vsnprintf() never returns a negative value To: Heinrich Schuchardt , Rasmus Villemoes Cc: U-Boot Mailing List , Tom Rini , Simon Glass References: <20210520100528.322846-1-rasmus.villemoes@prevas.dk> <20210520100528.322846-2-rasmus.villemoes@prevas.dk> <6d1761b1-5129-d5a1-24ba-a27d15c42198@gmx.de> From: Sean Anderson Message-ID: <7ea05a6b-2609-c082-acfc-e05f446c65c8@seco.com> Date: Fri, 21 May 2021 10:48:29 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 In-Reply-To: <6d1761b1-5129-d5a1-24ba-a27d15c42198@gmx.de> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [50.195.82.171] X-ClientProxiedBy: BLAPR03CA0023.namprd03.prod.outlook.com (2603:10b6:208:32b::28) To DB7PR03MB4523.eurprd03.prod.outlook.com (2603:10a6:10:19::27) MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from [172.27.1.65] (50.195.82.171) by BLAPR03CA0023.namprd03.prod.outlook.com (2603:10b6:208:32b::28) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4150.23 via Frontend Transport; Fri, 21 May 2021 14:48:32 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: cb204e9b-3876-4ec9-5ee7-08d91c6781b7 X-MS-TrafficTypeDiagnostic: DBBPR03MB7146: X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:3826; X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: cL913kXuhm0LFsxy3+oQ1MVPLzegGC3j26rmHExLzkh0DVII270LPoyCLv+dxUwrPNSfrEQtDrTsy7AhjS8h6NtH4B4n92KiYVlEATmmrW9wbOBc60sU2j3j5EUanCaD7YXjFHFklxsH0sbEoxq1Ye3PWKn6L3lEeeaMC6mqcJ3SDURFN1aorp12HgFNEx3IAXneGlGj2FYbvzA64bw22iIaeT+oHWx8a61dhFlZW3SZzFsrh/8/4e3yT+hfij7zSjWqSMRRxGN9Vob+JIK+2vA6cdVd9ZY+kU8fMAdMGnucW53MmKNbMSmP4PtRyjKZgBLx9ZBm9GWeg6s6Raq99nTavhpqmLFaaVirfq1By3OvlPgT8oqPtVkvBLoeeaCr97wHvuylVYrxjTXGmbjjeRQbsSKMqJLux7eVGTiwXIju/PwGgxsG68Ja01mE7LyAm1Q9WulSbR2cec9LQWa4pVvObE6TKjHN9RuMUZHgBGo1eHRvH9LXR9HsZpY0pXEQTc3NFUFy8QgFwl21QPadFv1Z2vm//x0CPzVwyyvM379wpW+PahVq4qTa+0rAu95/+los0AeBDwzIvVZRNF4ysCNDyNIQTZy2WDnTHXMGD+urzNQgJ+LCFlBZbdrtKFjaygEfQM6j5IfDDsJJ8tY25hE0OUDrCF1TIbvYt+jLPd0AGzy/mQj5sV7HGhYmqUykkPW85t481wY22V7KB5raEA== X-Forefront-Antispam-Report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:DB7PR03MB4523.eurprd03.prod.outlook.com; PTR:; CAT:NONE; SFS:(39840400004)(136003)(366004)(346002)(376002)(396003)(956004)(478600001)(16576012)(110136005)(6486002)(8936002)(316002)(36756003)(54906003)(53546011)(31686004)(66476007)(6666004)(26005)(4326008)(66946007)(2616005)(44832011)(86362001)(31696002)(66556008)(5660300002)(186003)(38350700002)(38100700002)(16526019)(83380400001)(2906002)(52116002)(8676002)(45980500001)(43740500002); DIR:OUT; SFP:1101; X-MS-Exchange-AntiSpam-MessageData: =?utf-8?B?NGdOQUlWUjFnR1hhZDVCTVk0NUNycHZYMllvNHF6MERqSEFpb2djVmhmS3B4?= =?utf-8?B?c2RHTnJqQnZuaHpmZUQvVEErWWw4dVdHOUtnRmtjayt3YkR0RU9zQnRWdXE2?= =?utf-8?B?bmZLZkF3djBLYUkyeWpJSlRFTXhGRWtFYjFZaU1QS2Y0akkwbTVIUDJpc3BS?= =?utf-8?B?WmgwQlhFUEJsWUZ1b25IVHFDa080VjF6LzBVUGdMSUlVLzRubUtKbkRqWkhP?= =?utf-8?B?ektReXlzRjg0ZWZhalMzMzJDa3l3dXRrUzVGMkV2ajhsSkZJT3Y1clJwSHpF?= =?utf-8?B?OVBtWjVJQVFnNGtBMnozODdLcG8yOURveGdkNzFnZVZsNWFCazZZUkFsbEVl?= =?utf-8?B?b0N6ekQ1a0gwcU1DdHFwZjBXdnNjV1FTVUk0cGNkWmNILzRrUml2QlMybmlX?= =?utf-8?B?RURKNUUzRFJoS21qUDRSaGR1ZUZRN09TWkdBNWxucnFPZXM0R1JxdVN3WFNt?= =?utf-8?B?R1dSWjFyNHdLWFdrclRhTXJYOTZUY2FKV3dOY3pkYTUxZTFmVnd1T0JhWlp6?= =?utf-8?B?VlNObnQwQWM0QUpKZkhvejFBVjhzS2NCL2pVd2RONlJFZFIweXRKMkpjTzZm?= =?utf-8?B?UHRnOExGcnBFRVZGR1prOThIcDJ1OFB5WVRaUDhkZ0hCQXZ5T3BJUi9KbVFw?= =?utf-8?B?Nk9NMkZZSWZoS0tyZUhSb0JDWkMrVXVIbGNmK2xPN0ZGK2hnUU84dzAwRFFK?= =?utf-8?B?blFZZElFeXhQY2ZOeXNDcDY1dUEvMjF4MS8xUngvUTZvTTJqdlVxQmZDY2k0?= =?utf-8?B?VlQ0bFo0dndMOXdYekoxamgyZWdSbkJtTnZCZ3lKTE9QV3JzVDUybkxDR1hI?= =?utf-8?B?WFVocU9Gd2JHRDR2czBGaEVTUW56TUNlQjdjRjBwVkE3V1JQY2ZRNkRHeU9q?= =?utf-8?B?dGk5SzkrZXZoajVzKzgyb2YxaXViTmpvOEs3MWJIcnJoamh0WjB3REgvb3NF?= =?utf-8?B?Nk84ejNKNVpCZ0lRK3BkQUZ3Nmo0UU1wTVIrYytEenpPK3BUbmdzaytZRFN5?= =?utf-8?B?ZXhkK3NpeXRGc1YrS0tYSzhXa1F1dm1FUzdBNnhrK1F2NTAyZVVHeklWd0Jv?= =?utf-8?B?cmRwTGlHaTdzYkg0RUZTRXJjcitTNXZ5TkVTTng2VkhGMkZ2NmJRd2pFZE5D?= =?utf-8?B?TXk4TmgvbkpiL1FQRDVDMmhyRkJhbTB3bzdwazJhcnlyOWRpZG42NUhDQ2U2?= =?utf-8?B?bGNENlVId29wYU5OVDNneDhjYytYTnRuVTlFK1libWRmTk84UlRsVE1WNmc2?= =?utf-8?B?NUkxRTRTazQwd2tuNzhoVDk5NEM4ejU5enczUmxxTDRVMjJFSXpIeVNxd09r?= =?utf-8?B?RkxoaExqd3hIQTFycExwc1d1VHVzdExwZ3dSd2ZOR1U2ODZ4WUZ1bm8vMHNG?= =?utf-8?B?SUtxQlFlZGs0eTh4S2pxR3RaRmQ2MFlOeVg5N1ZYdkZuR1pxMDRYYmNCZ01W?= =?utf-8?B?VWNWY2hUbUtxQmI4NXhTdGhURDQ5TllmYkFQL1VEOTVMbk5DS0tuYURmUm12?= =?utf-8?B?SkVIZXk5aFRDZUZCY254aUp3YjFZdVQzT1hCdVBJaklJR0VHaFZjam9XM0l2?= =?utf-8?B?ODFMMitwcVcvcUVsazNWeUdlbHlNUGRaZkx2S0ZKa0VHMjlzOHdEdzgrS3Bq?= =?utf-8?B?Rnh6WkNQVWoyNDQ2Q1ZLTEJsQ2E1Y0grUm1wdXVkM3pRV2w5dnhCd0U1Vmx1?= =?utf-8?B?TUpyeS9UeDYwb0pBZVNEUnpXeGdWTkY1ajN1c0U2Z214Ymt2cHhhVFVyeDZ3?= =?utf-8?Q?8S962z5dkHWIAjj3P1Yh262OJoqa8dY8P7GVY+x?= X-OriginatorOrg: seco.com X-MS-Exchange-CrossTenant-Network-Message-Id: cb204e9b-3876-4ec9-5ee7-08d91c6781b7 X-MS-Exchange-CrossTenant-AuthSource: DB7PR03MB4523.eurprd03.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 May 2021 14:48:33.4562 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: bebe97c3-6438-442e-ade3-ff17aa50e733 X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: EArPl71hUfmfLDTLgSAjFN7NwgjWkxPoKApvyWr9WOhDNZLo45NExznc57Z//1NmzMhiKjfZy4I9JzoFUIP5Yg== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DBBPR03MB7146 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 5/21/21 10:15 AM, 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 some >>>> "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 have > been written had n been sufficiently large, not counting the > terminating null character, or a negative value if an encoding error > occurred." But is this an encoding error? And of course, the most common use of this function which checks this return value will be n = vsnprintf(NULL, 0, fmt, ...); buf = malloc(n); if (!buf) /* out of memory */ vsnprintf(buf, n, fmt, ...); Which effectively already checks for ENOMEM. While it is standard-compliant to return negative, it is also compliant to just use a bogus value or to write nothing. There are many callers of [v]s[c]nprintf which use the same pattern as the code above. If you would like to fix all of them, go ahead. But I think this fix is more efficient a cut. --Sean > > 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 places >> 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 the >> normal userspace version) obviously can fail for the obvious EIO, ENOSPC >> 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 indirect >> 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 so >> that it ignores the message if vsnprintf (or vscnprintf, whatever) >> returns a negative result, just as print() currently does [which is the >> 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 out, >> 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 should >> 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 epilogue >> that ensures the resulting string becomes nul-terminated (another >> reasonable assumption made by tons of callers), and return how much got >> printed till then. >> >> Rasmus >> >