From patchwork Tue Jun 20 17:58:55 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Woodhouse X-Patchwork-Id: 1797497 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=nongnu.org (client-ip=209.51.188.17; helo=lists.gnu.org; envelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org; receiver=) Authentication-Results: legolas.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; secure) header.d=infradead.org header.i=@infradead.org header.a=rsa-sha256 header.s=casper.20170209 header.b=ZpLlOTsO; dkim-atps=neutral Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-ECDSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4QlvXn4k7Bz20Wk for ; Wed, 21 Jun 2023 03:59:37 +1000 (AEST) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qBfdR-0003kD-NL; Tue, 20 Jun 2023 13:59:21 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qBfdQ-0003k4-LP for qemu-devel@nongnu.org; Tue, 20 Jun 2023 13:59:20 -0400 Received: from casper.infradead.org ([2001:8b0:10b:1236::1]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qBfd4-0006VH-Pm for qemu-devel@nongnu.org; Tue, 20 Jun 2023 13:59:10 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=MIME-Version:Content-Type:References: In-Reply-To:Date:Cc:To:From:Subject:Message-ID:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=P0rw6j/526CasCXlDjY88i1DcnH89PB7+0min8yStY8=; b=ZpLlOTsOcbygS6lck1zF0UH1KD 7SjmlMugZb9keYTZLWbAHAqdyc2E862zUAcmk3+QRKEnmpQeVatOJ8mhnaG5Gt2x2Yn0M02uue5Th eIHFDzbUgdBN8bZy+AlB5tNskU5tWsvKjVrcqflIRVmYovUwlOYGVk8iWCDVZdI6x6nKfuPcy8ep3 rrM8p+0rswr4dpkm2UyLJegesLxB1x/us6NAXpNBKo8LF71YJ2L0tGViZHEM0p/Vu/04l11PjnKmE 1WU0SlNUj2AQ1/iIqNm/l/GYS8+eGQNUScPm+d8vW1rsc2DPjhPDkpIUXhHTG+KbkhsVX2cqNlVdY TspDN2Tw==; Received: from [2001:8b0:10b:5:f26b:798c:7de3:5106] (helo=u3832b3a9db3152.ant.amazon.com) by casper.infradead.org with esmtpsa (Exim 4.94.2 #2 (Red Hat Linux)) id 1qBfd2-00DNGy-1q; Tue, 20 Jun 2023 17:58:56 +0000 Message-ID: <20076888f6bdf06a65aafc5cf954260965d45b97.camel@infradead.org> Subject: [PATCH] hw/xen: Clarify (lack of) error handling in transaction_commit() From: David Woodhouse To: Peter Maydell Cc: qemu-devel@nongnu.org, Paolo Bonzini , Paul Durrant , Joao Martins , Ankur Arora , Stefano Stabellini , vikram.garhwal@amd.com, Anthony Perard , xen-devel@lists.xenproject.org, Juan Quintela , "Dr . David Alan Gilbert" Date: Tue, 20 Jun 2023 18:58:55 +0100 In-Reply-To: References: <20230307182707.2298618-1-dwmw2@infradead.org> <20230307182707.2298618-6-dwmw2@infradead.org> User-Agent: Evolution 3.44.4-0ubuntu1 MIME-Version: 1.0 X-SRS-Rewrite: SMTP reverse-path rewritten from by casper.infradead.org. See http://www.infradead.org/rpr.html Received-SPF: none client-ip=2001:8b0:10b:1236::1; envelope-from=BATV+288f744f4c934e53f325+7240+infradead.org+dwmw2@casper.srs.infradead.org; helo=casper.infradead.org X-Spam_score_int: -43 X-Spam_score: -4.4 X-Spam_bar: ---- X-Spam_report: (-4.4 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_MED=-2.3, SPF_HELO_NONE=0.001, SPF_NONE=0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org From: David Woodhouse Coverity was unhappy (CID 1508359) because we didn't check the return of init_walk_op() in transaction_commit(), despite doing so at every other call site. Strictly speaking, this is a false positive since it can never fail. It only fails for invalid user input (transaction ID or path), and both of those are hard-coded to known sane values in this invocation. But Coverity doesn't know that, and neither does the casual reader of the code. Returning an error here would be weird, since the transaction *is* committed by this point; all the walk_op is doing is firing watches on the newly-committed changed nodes. So make it a g_assert(!ret), since it really should never happen. Signed-off-by: David Woodhouse Reviewed-by: Paul Durrant --- hw/i386/kvm/xenstore_impl.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/hw/i386/kvm/xenstore_impl.c b/hw/i386/kvm/xenstore_impl.c index 305fe75519..d9732b567e 100644 --- a/hw/i386/kvm/xenstore_impl.c +++ b/hw/i386/kvm/xenstore_impl.c @@ -1022,6 +1022,7 @@ static int transaction_commit(XenstoreImplState *s, XsTransaction *tx) { struct walk_op op; XsNode **n; + int ret; if (s->root_tx != tx->base_tx) { return EAGAIN; @@ -1032,7 +1033,16 @@ static int transaction_commit(XenstoreImplState *s, XsTransaction *tx) s->root_tx = tx->tx_id; s->nr_nodes = tx->nr_nodes; - init_walk_op(s, &op, XBT_NULL, tx->dom_id, "/", &n); + ret = init_walk_op(s, &op, XBT_NULL, tx->dom_id, "/", &n); + /* + * There are two reasons why init_walk_op() may fail: an invalid tx_id, + * or an invalid path. We pass XBT_NULL and "/", and it cannot fail. + * If it does, the world is broken. And returning 'ret' would be weird + * because the transaction *was* committed, and all this tree walk is + * trying to do is fire the resulting watches on newly-committed nodes. + */ + g_assert(!ret); + op.deleted_in_tx = false; op.mutating = true;