From patchwork Tue Aug 16 11:23:54 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jan Venekamp X-Patchwork-Id: 1666804 X-Patchwork-Delegate: hauke@hauke-m.de Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=none (no SPF record) smtp.mailfrom=lists.openwrt.org (client-ip=2607:7c80:54:3::133; helo=bombadil.infradead.org; envelope-from=openwrt-devel-bounces+incoming=patchwork.ozlabs.org@lists.openwrt.org; receiver=) Authentication-Results: legolas.ozlabs.org; dkim=pass (2048-bit key; secure) header.d=lists.infradead.org header.i=@lists.infradead.org header.a=rsa-sha256 header.s=bombadil.20210309 header.b=S4+znJqs; dkim-atps=neutral Received: from bombadil.infradead.org (bombadil.infradead.org [IPv6:2607:7c80:54:3::133]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4M6TQg5GH8z1ygF for ; Tue, 16 Aug 2022 21:27:39 +1000 (AEST) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-Id:Date:Subject:To:From:Reply-To:Cc:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=Y/ivmo7KYZUehGH0ffyfyYXuYAasZxfEWmiUh20wxfM=; b=S4+znJqssnS/el yVxrMfaYHM2HCVkK97WqS15Jg9pmuTMBCp1tJe0ld7pwHy2P8mCyUFiqvbPa3/pDCEa+AV6oxVDHN LJrLIFsjlnFgWK8bfkfHqBWREYXxEmF9SHAH8dHk4t6/3gh9VUTU5v71FnLlQMkMGEhKAwcw5E4Bl pMIyuwJgRiTxoLOX9rXll6VCfh845Xk9W0Wu62OamW2J6ps8N3y6kCYnfMMJhsfzxRGE3NDpX6mvi 4Aq3oJa0vDxpLS6flLV9p9NOIbUTsHIhOpwNaleroWeV7Mx7/8W3xrZmesT6ZFLU0xmbGEgdonNqY 6TGAdu9c63CoDJb1IZAg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oNuhl-001X4i-9r; Tue, 16 Aug 2022 11:25:53 +0000 Received: from virt1.bvwebdesign.nl ([149.210.228.112]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oNug5-001W6x-KN for openwrt-devel@lists.openwrt.org; Tue, 16 Aug 2022 11:24:11 +0000 Received: from localhost.localdomain (84-31-67-158.cable.dynamic.v4.ziggo.nl [84.31.67.158]) by virt1.bvwebdesign.nl (Postfix) with ESMTPSA id C07CD964542 for ; Tue, 16 Aug 2022 13:23:59 +0200 (CEST) From: Jan Venekamp To: openwrt-devel@lists.openwrt.org Subject: [PATCH 5/9] uci: fix atomicity of uci_add_list Date: Tue, 16 Aug 2022 13:23:54 +0200 Message-Id: <20220816112358.75801-6-jan@venekamp.net> X-Mailer: git-send-email 2.32.0 (Apple Git-132) In-Reply-To: <20220816112358.75801-1-jan@venekamp.net> References: <20220816112358.75801-1-jan@venekamp.net> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220816_042410_015916_57C38AEB X-CRM114-Status: GOOD ( 13.15 ) X-Spam-Score: -0.0 (/) X-Spam-Report: Spam detection software, running on the system "bombadil.infradead.org", has NOT identified this incoming email as spam. The original message has been attached to this so you can view it or label similar future email. If you have any questions, see the administrator of that system for details. Content preview: The function uci_add_list is not atomic, when an alloc inside uci_add_element_list fails the option can be left in an indeterminate state. Refactor uci_add_list to fix this and make the code flow easier to read. Content analysis details: (-0.0 points, 5.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- -0.0 SPF_PASS SPF: sender matches SPF record -0.0 SPF_HELO_PASS SPF: HELO matches SPF record X-BeenThere: openwrt-devel@lists.openwrt.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: OpenWrt Development List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "openwrt-devel" Errors-To: openwrt-devel-bounces+incoming=patchwork.ozlabs.org@lists.openwrt.org The function uci_add_list is not atomic, when an alloc inside uci_add_element_list fails the option can be left in an indeterminate state. Refactor uci_add_list to fix this and make the code flow easier to read. Signed-off-by: Jan Venekamp --- list.c | 74 +++++++++++++++++++++++++++++----------------------------- 1 file changed, 37 insertions(+), 37 deletions(-) diff --git a/list.c b/list.c index ba099b6..3518967 100644 --- a/list.c +++ b/list.c @@ -497,19 +497,6 @@ uci_expand_ptr(struct uci_context *ctx, struct uci_ptr *ptr, bool complete) return NULL; } -static void uci_add_element_list(struct uci_context *ctx, struct uci_ptr *ptr, bool internal) -{ - struct uci_element *e; - struct uci_package *p; - - p = ptr->p; - if (!internal && p->has_delta) - uci_add_delta(ctx, &p->delta, UCI_CMD_LIST_ADD, ptr->section, ptr->option, ptr->value); - - e = uci_alloc_generic(ctx, UCI_TYPE_ITEM, ptr->value, sizeof(struct uci_option)); - uci_list_add(&ptr->o->v.list, &e->list); -} - int uci_rename(struct uci_context *ctx, struct uci_ptr *ptr) { /* NB: UCI_INTERNAL use means without delta tracking */ @@ -623,8 +610,7 @@ int uci_add_list(struct uci_context *ctx, struct uci_ptr *ptr) { /* NB: UCI_INTERNAL use means without delta tracking */ bool internal = ctx && ctx->internal; - struct uci_option *volatile prev = NULL; - const char *volatile value2 = NULL; + struct uci_element *volatile e1 = NULL, *volatile e2 = NULL; UCI_HANDLE_ERR(ctx); @@ -632,34 +618,48 @@ int uci_add_list(struct uci_context *ctx, struct uci_ptr *ptr) UCI_ASSERT(ctx, ptr->s); UCI_ASSERT(ctx, ptr->value); - if (ptr->o) { - switch (ptr->o->type) { - case UCI_TYPE_STRING: - /* we already have a string value, convert that to a list */ - prev = ptr->o; - value2 = ptr->value; - ptr->value = ptr->o->v.string; - break; - case UCI_TYPE_LIST: - uci_add_element_list(ctx, ptr, internal); - return 0; - default: - UCI_THROW(ctx, UCI_ERR_INVAL); - break; - } + if (ptr->o && ptr->o->type != UCI_TYPE_LIST && ptr->o->type != UCI_TYPE_STRING) { + UCI_THROW(ctx, UCI_ERR_INVAL); } - ptr->o = uci_alloc_list(ptr->s, ptr->option); - if (prev) { - uci_add_element_list(ctx, ptr, true); - if (ptr->option == prev->e.name) + /* create new item */ + e1 = uci_alloc_generic(ctx, UCI_TYPE_ITEM, ptr->value, sizeof(struct uci_option)); + + if (!ptr->o) { + /* create new list */ + UCI_TRAP_SAVE(ctx, error); + ptr->o = uci_alloc_list(ptr->s, ptr->option); + UCI_TRAP_RESTORE(ctx); + ptr->last = &ptr->o->e; + } else if (ptr->o->type == UCI_TYPE_STRING) { + /* create new list and add old string value as item to list */ + struct uci_option *old = ptr->o; + UCI_TRAP_SAVE(ctx, error); + e2 = uci_alloc_generic(ctx, UCI_TYPE_ITEM, old->v.string, sizeof(struct uci_option)); + ptr->o = uci_alloc_list(ptr->s, ptr->option); + UCI_TRAP_RESTORE(ctx); + uci_list_add(&ptr->o->v.list, &e2->list); + + /* remove old option */ + if (ptr->option == old->e.name) ptr->option = ptr->o->e.name; - uci_free_option(prev); - ptr->value = value2; + uci_free_option(old); + ptr->last = &ptr->o->e; } - uci_add_element_list(ctx, ptr, internal); + + /* add new item to list */ + uci_list_add(&ptr->o->v.list, &e1->list); + + if (!internal && ptr->p->has_delta) + uci_add_delta(ctx, &ptr->p->delta, UCI_CMD_LIST_ADD, ptr->section, ptr->option, ptr->value); return 0; +error: + if (e1 != NULL) + uci_free_element(e1); + if (e2 != NULL) + uci_free_element(e2); + UCI_THROW(ctx, ctx->err); } int uci_del_list(struct uci_context *ctx, struct uci_ptr *ptr)