From patchwork Tue Aug 16 11:23:56 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jan Venekamp X-Patchwork-Id: 1666805 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=lhOtzOB8; 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 4M6TRZ4hzlz1ygF for ; Tue, 16 Aug 2022 21:28:26 +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=H7smgNbuq/TI1mujoB1lwVISY30eiWCPPiRAiqS+IRc=; b=lhOtzOB8BQEEek MpY2ZElt6z6SikFmTVeWr4Aj+3jmhX7mrUCg9RQobV9QICAfxZrd5X568Z/Acvh+TS/Bk0GjOaaiM T4oOXn5nlM62c5JeQmB0IhLtYac2ZKIUReZO746UCr+tLsVBwbExYxDJbej8gWzucwFtaSbz+s07I ijhVxg0FlhFJ62e6RZiUSA92WNZ2BGXK61XabhtugMm/lpdc4T1Ep91SGETDU23wrKnJ2WwSylLey h2dx+abovA4hvJB7ADK5xT9zkiAz6GzGRiEiYIKro0pKWytQJSFG7QEpx8ucgOpPEg9+vx4GEE2Vg EsOXpvhyTL3jWXmsythQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oNuiN-001XUS-MP; Tue, 16 Aug 2022 11:26:32 +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-001W70-MC for openwrt-devel@lists.openwrt.org; Tue, 16 Aug 2022 11:24:12 +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 E90B3964544 for ; Tue, 16 Aug 2022 13:23:59 +0200 (CEST) From: Jan Venekamp To: openwrt-devel@lists.openwrt.org Subject: [PATCH 7/9] uci: fix memory leak uci_set on update section Date: Tue, 16 Aug 2022 13:23:56 +0200 Message-Id: <20220816112358.75801-8-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_078370_43F54FB1 X-CRM114-Status: GOOD ( 19.04 ) 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: If uci_realloc fails when updating a section in uci_set the reference to the memory allocated by s = uci_strdup() is lost. Also, if uci_strdup and uci_realloc both succeed it could happen that ptr->s->type == uci_dataptr(ptr->s) by accident. Then later on in uci_free_section the allocated ptr->s->type is not freed. 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 If uci_realloc fails when updating a section in uci_set the reference to the memory allocated by s = uci_strdup() is lost. Also, if uci_strdup and uci_realloc both succeed it could happen that ptr->s->type == uci_dataptr(ptr->s) by accident. Then later on in uci_free_section the allocated ptr->s->type is not freed. In order to fix this, instead of splitting the allocation of the the section and the type string, we create a new section with in-section storage to replace the old one. This also brings the code for updating a section more in line with the simular code for updating an option. Signed-off-by: Jan Venekamp --- list.c | 62 +++++++++++++++++++--------------------------------------- 1 file changed, 20 insertions(+), 42 deletions(-) diff --git a/list.c b/list.c index e6d631c..783836b 100644 --- a/list.c +++ b/list.c @@ -30,12 +30,6 @@ static bool uci_list_set_pos(struct uci_list *head, struct uci_list *ptr, int po return (old_head != new_head); } -static inline void uci_list_fixup(struct uci_list *ptr) -{ - ptr->prev->next = ptr; - ptr->next->prev = ptr; -} - /* * uci_alloc_generic allocates a new uci_element with payload * payload is appended to the struct to save memory and reduce fragmentation @@ -182,34 +176,26 @@ static void uci_fixup_section(struct uci_context *ctx, struct uci_section *s) s->e.name = uci_strdup(ctx, buf); } -/* fix up option list HEAD pointers and pointer to section in options */ -static void uci_section_fixup_options(struct uci_section *s, bool no_options) +/* transfer options between two sections */ +static void uci_section_transfer_options(struct uci_section *dst, struct uci_section *src) { struct uci_element *e; - if (no_options) { - /* - * enforce empty list pointer state (s->next == s) when original - * section had no options in the first place - */ - uci_list_init(&s->options); - return; - } - - /* fix pointers to HEAD at end/beginning of list */ - uci_list_fixup(&s->options); + /* transfer the option list by inserting the new list HEAD and removing the old */ + uci_list_insert(&src->options, &dst->options); + uci_list_del(&src->options); - /* fix back pointer to section in options */ - uci_foreach_element(&s->options, e) { + /* update pointer to section in options */ + uci_foreach_element(&dst->options, e) { struct uci_option *o; o = uci_to_option(e); - o->section = s; + o->section = dst; } } static struct uci_section * -uci_alloc_section(struct uci_package *p, const char *type, const char *name) +uci_alloc_section(struct uci_package *p, const char *type, const char *name, struct uci_list *after) { struct uci_context *ctx = p->ctx; struct uci_section *s; @@ -226,7 +212,7 @@ uci_alloc_section(struct uci_package *p, const char *type, const char *name) s->anonymous = true; p->n_section++; - uci_list_add(&p->sections, &s->e.list); + uci_list_insert(after ? after : p->sections.prev, &s->e.list); return s; } @@ -551,7 +537,7 @@ int uci_add_section(struct uci_context *ctx, struct uci_package *p, const char * UCI_HANDLE_ERR(ctx); UCI_ASSERT(ctx, p != NULL); - s = uci_alloc_section(p, type, NULL); + s = uci_alloc_section(p, type, NULL, NULL); if (s && s->anonymous) uci_fixup_section(ctx, s); *res = s; @@ -724,7 +710,7 @@ int uci_set(struct uci_context *ctx, struct uci_ptr *ptr) ptr->o = uci_alloc_option(ptr->s, ptr->option, ptr->value, NULL); ptr->last = &ptr->o->e; } else if (!ptr->s && ptr->section) { /* new section */ - ptr->s = uci_alloc_section(ptr->p, ptr->value, ptr->section); + ptr->s = uci_alloc_section(ptr->p, ptr->value, ptr->section, NULL); ptr->last = &ptr->s->e; } else if (ptr->o && ptr->option) { /* update option */ if (ptr->o->type == UCI_TYPE_STRING && !strcmp(ptr->o->v.string, ptr->value)) @@ -741,22 +727,14 @@ int uci_set(struct uci_context *ctx, struct uci_ptr *ptr) ptr->last = &ptr->o->e; } } else if (ptr->s && ptr->section) { /* update section */ - char *s = uci_strdup(ctx, ptr->value); - - if (ptr->s->type == uci_dataptr(ptr->s)) { - /* drop the in-section storage of type name */ - bool no_options; - - no_options = uci_list_empty(&ptr->s->options); - ptr->last = NULL; - ptr->last = uci_realloc(ctx, ptr->s, sizeof(struct uci_section)); - ptr->s = uci_to_section(ptr->last); - uci_list_fixup(&ptr->s->e.list); - uci_section_fixup_options(ptr->s, no_options); - } else { - free(ptr->s->type); - } - ptr->s->type = s; + struct uci_section *old = ptr->s; + ptr->s = uci_alloc_section(ptr->p, ptr->value, ptr->section, &old->e.list); + uci_section_transfer_options(ptr->s, old); + if (ptr->section == old->e.name) + ptr->section = ptr->o->e.name; + uci_free_section(old); + ptr->s->package->n_section--; + ptr->last = &ptr->s->e; } else { UCI_THROW(ctx, UCI_ERR_INVAL); }