Re: [PATCH uci] uci: decrease the n_section when section is freed

[Jan Venekamp]

On 14/09/2023 23:36, Leon Busch-George wrote:
> Before finding and applying Jeff's patch I had written a small test
> application that creates a few random sections in the 'dhcp' UCI
> package and deletes them again (uci_set).
> Afterwards, it iterates all packages (uci_list_configs), prints a line
> if there is a delta (ptr.p->has_delta), and calls uci_save.
> Basically, there were erroneous 'has_delta' entries for all packages.

The bool package->has_delta is used by libuci to determine whether to
use "delta tracking". This value is set to true for every package that
is loaded from a file within confdir (default: /etc/config),
see uci_load() -> uci_file_load(). So yeah, if you loaded the packages
from within /etc/config it would be true for every package.

> If enough sections were created and deleted, I would reliably segfault
> when iterating sections (e.g.
> uci_foreach_element_safe(_ptr.p->sections, tmp, e)).
> That is how I discovered the bug.

I am very curious as to how Jeff's patch helps with that, because the
package->n_section value is only used to auto-create a name for
anonymous sections.

> I abandonded and deleted the test app and the report since, at least
> to me, Jeff's patch appears to be a valid solution.
> If you want, I could recreate it. Let me know!

Please do. If you could provide example code, I will gladly take a
look at it.

___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

[PATCH uci] reduce code complexity of uci_show_package

[Jan Venekamp]

For the extended syntax on "uci show" a list of previous section types
is build and iterated over to determine the current type index of a
section. However, instead building this list and iterate over that we
can simply iterate over the previous sections to find the index. This
reduces the code complexity of uci_show_package, uci_show_section and
uci_show_option.

Signed-off-by: Jan Venekamp 
---
 cli.c | 132 +-
 .../references/show_parsing_multiline.data|   3 +
 .../show_parsing_multiline_package.result |   2 +
 3 files changed, 37 insertions(+), 100 deletions(-)

diff --git a/cli.c b/cli.c
index f169e42..2cbfecf 100644
--- a/cli.c
+++ b/cli.c
@@ -55,88 +55,8 @@ enum {
CMD_HELP,
 };
 
-struct uci_type_list {
-   unsigned int idx;
-   const char *name;
-   struct uci_type_list *next;
-};
-
-static struct uci_type_list *type_list = NULL;
-static char *typestr = NULL;
-static const char *cur_section_ref = NULL;
-
 static int uci_cmd(int argc, char **argv);
 
-static void
-uci_reset_typelist(void)
-{
-   struct uci_type_list *type;
-   while (type_list != NULL) {
-   type = type_list;
-   type_list = type_list->next;
-   free(type);
-   }
-   if (typestr) {
-   free(typestr);
-   typestr = NULL;
-   }
-   cur_section_ref = NULL;
-}
-
-static char *
-uci_lookup_section_ref(struct uci_section *s)
-{
-   struct uci_type_list *ti = type_list;
-   char *ret;
-   int maxlen;
-
-   if (!(flags & CLI_FLAG_SHOW_EXT))
-   return s->e.name;
-
-   /* look up in section type list */
-   while (ti) {
-   if (strcmp(ti->name, s->type) == 0)
-   break;
-   ti = ti->next;
-   }
-   if (!ti) {
-   ti = calloc(1, sizeof(struct uci_type_list));
-   if (!ti)
-   return NULL;
-   ti->next = type_list;
-   type_list = ti;
-   ti->name = s->type;
-   }
-
-   if (s->anonymous) {
-   maxlen = strlen(s->type) + 1 + 2 + 10;
-   if (!typestr) {
-   typestr = malloc(maxlen);
-   if (!typestr)
-   return NULL;
-   } else {
-   void *p = realloc(typestr, maxlen);
-   if (!p) {
-   free(typestr);
-   return NULL;
-   }
-
-   typestr = p;
-   }
-
-   if (typestr)
-   sprintf(typestr, "@%s[%d]", ti->name, ti->idx);
-
-   ret = typestr;
-   } else {
-   ret = s->e.name;
-   }
-
-   ti->idx++;
-
-   return ret;
-}
-
 static void uci_usage(void)
 {
fprintf(stderr,
@@ -243,40 +163,52 @@ static void uci_show_value(struct uci_option *o, bool 
quote)
}
 }
 
-static void uci_show_option(struct uci_option *o, bool quote)
+static void uci_show_option(struct uci_option *o)
 {
-   printf("%s.%s.%s=",
-   o->section->package->e.name,
-   (cur_section_ref ? cur_section_ref : o->section->e.name),
-   o->e.name);
-   uci_show_value(o, quote);
+   printf("%s.%s.%s=", o->section->package->e.name, o->section->e.name, 
o->e.name);
+   uci_show_value(o, true);
 }
 
 static void uci_show_section(struct uci_section *s)
 {
struct uci_element *e;
-   const char *cname;
-   const char *sname;
 
-   cname = s->package->e.name;
-   sname = (cur_section_ref ? cur_section_ref : s->e.name);
-   printf("%s.%s=%s\n", cname, sname, s->type);
+   printf("%s.%s=%s\n", s->package->e.name, s->e.name, s->type);
uci_foreach_element(>options, e) {
-   uci_show_option(uci_to_option(e), true);
+   uci_show_option(uci_to_option(e));
}
 }
 
 static void uci_show_package(struct uci_package *p)
 {
-   struct uci_element *e;
+   struct uci_element *e0, *e1;
+   bool show_ext = flags & CLI_FLAG_SHOW_EXT;
+
+   uci_foreach_element(>sections, e0) {
+   struct uci_section *s = uci_to_section(e0);
+   unsigned int idx = 0;
+
+   if (!s->anonymous || !show_ext) {
+   uci_show_section(s);
+   continue;
+   }
 
-   uci_reset_typelist();
-   uci_foreach_element( >sections, e) {
-   struct uci_section *s = uci_to_section(e);
-   cur_section_ref = uci_lookup_section_ref(s);
-   uci_show_section(s);
+   /* count preceding sections with same type */

Re: [PATCH netifd] system-linux: Do not unconditionally activate IPv6 on devices

[Jan Venekamp]

I overlooked this patch when submitting [1]. Slightly different argumentation,
exact same conclusion.

Hauke can you apply this?

Kind regards,
Jan Venekamp

[1] 
https://patchwork.ozlabs.org/project/openwrt/patch/20230626193755.25155-1-...@venekamp.net/

Reviewed-by: Jan Venekamp 

___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

Re: [PATCH uci] uci: decrease the n_section when section is freed

[Jan Venekamp]

When working on 16e8a3b1 [1] I looked at anonymous sections and
uci_fixup_section too. It seemed rather hairy to me, but I decided then
to leave it as is.

> The package n_section counter increases when a section is allocated
> but does not decrease when a section is freed.

I suspect this is by design. Find attached a test case that your
patch does not pass.

> Since the anonymous section name is comprised of the section counter,
> if the package is not reloaded, an incorrect count will result in
> operating on the wrong section.

Could you provided a example / test case for this behaviour?

Kind regards,
Jan Venekamp

[1] https://git.openwrt.org/?p=project/uci.git;a=commitdiff;h=16e8a3b1

---
 .../references/batch_anonymous_section.result   |  7 +++
 tests/shunit2/tests.d/060_batch | 17 +
 2 files changed, 24 insertions(+)
 create mode 100644 tests/shunit2/references/batch_anonymous_section.result

diff --git a/tests/shunit2/references/batch_anonymous_section.result 
b/tests/shunit2/references/batch_anonymous_section.result
new file mode 100644
index 000..fe000a1
--- /dev/null
+++ b/tests/shunit2/references/batch_anonymous_section.result
@@ -0,0 +1,7 @@
+
+config foo
+   option bar '1'
+
+config foo
+   option bar '2'
+
diff --git a/tests/shunit2/tests.d/060_batch b/tests/shunit2/tests.d/060_batch
index 40f473b..4aa3047 100644
--- a/tests/shunit2/tests.d/060_batch
+++ b/tests/shunit2/tests.d/060_batch
@@ -41,3 +41,20 @@ EOF
 
assertSameFile "${REF_DIR}/batch_comments.result" 
"${CONFIG_DIR}/batch_comments"
 }
+
+test_batch_anonymous_section()
+{
+   touch ${CONFIG_DIR}/batch_anonymous_section
+
+${UCI} batch <https://lists.openwrt.org/mailman/listinfo/openwrt-devel

Re: [PATCH uci 2/2] remove internal usage of redundant uci_ptr.last

[Jan Venekamp]

On 01/08/2023 22:27, Hauke Mehrtens wrote:

On 7/14/23 20:28, Jan Venekamp wrote:

In uci_lookup_ptr and uci_set the pointer uci_ptr ptr.last is set to
the element corresponding to the first of: ptr.o, ptr.s, ptr.p.

Thus, ptr.last is redundant and in case of uci_set is (and was) not
always consistently set.

In order to simplify the code this commit removes internal usage
of ptr.last, and remove setting it from uci_set (and from uci_add_list
that was never used anyway).

As it is part of the public C api ptr.last cannot be completely
removed though. A search on lxr.openwrt.org shows that it is used as
the output of uci_lookup_ptr in several packages.

So we leave setting ptr.last in uci_lookup_ptr intact.

Signed-off-by: Jan Venekamp 
---
  cli.c | 39 +++
  delta.c   | 10 ++
  list.c    |  6 --
  lua/uci.c | 42 +++---
  4 files changed, 32 insertions(+), 65 deletions(-)


..

@@ -349,20 +347,14 @@ static int package_cmd(int cmd, char *tuple)
  cli_perror();
  goto out;
  }
-    switch(e->type) {
-    case UCI_TYPE_PACKAGE:
-    uci_show_package(ptr.p);
-    break;
-    case UCI_TYPE_SECTION:
-    uci_show_section(ptr.s);
-    break;
-    case UCI_TYPE_OPTION:
-    uci_show_option(ptr.o, true);
-    break;
-    default:
-    /* should not happen */
-    goto out;
-    }
+    if (ptr.o)
+    uci_show_option(ptr.o, true);
+    else if (ptr.s)
+    uci_show_section(ptr.s);
+    else if (ptr.p)
+    uci_show_package(ptr.p);
+    else
+    goto out; /* should not happen */
  break;


How do we make sure that only one of these is set at a time?
Before the code would print the last element added.



The code in uci_lookup_ptr makes sure that the element being looked up 
is the last of: ptr.p, ptr.s, ptr.o with a non null value (pointer).


Thus, if an option is looked up ptr.o is set and printed. When a section 
is looked up ptr.o is null, but ptr.s is set and printed. Else a package 
is looked up, ptr.o and ptr.s are null so ptr.p is printed. (When a 
looked up element is not found UCI_LOOKUP_COMPLETE is not set which will 
result in an error.)


So the code prints the same element as before.

Hope this explains it.


  }
  @@ -475,7 +467,6 @@ done:

..



___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

Re: [PATCH uci] file: Fix uci -m import command

[Jan Venekamp]

On 13/07/2023 18:53, Hauke Mehrtens wrote:

Without this change we see the following error:
   # uci -m import optic < /etc/optic-db/default
   uci: Parse error (option/list command found before the first section) at 
line 4, byte 1

ptr.last is still a null pointer in case the uci_lookup_list() call
found a matching section and set ptr.s to it. The code expects that
uci_set() updates the ptr.last pointer, but this is not done any more


Ah snap, I overlooked that in uci_parse_config while working on uci_set.

A clearer fix might be to just change the last line of uci_parse_config to:
pctx->section = ptr.s;
The change to uci_parse_option does not do anything, ptr->last is never 
read after being set.


However, seeing this made me more convinced that ptr.last just 
complicates the code while being redundant anyway. So I created a patch 
that removes internal usage of ptr.last altogether. Would you please be 
so kind to take a look at it?

https://patchwork.ozlabs.org/project/openwrt/list/?series=363985

While your at it, maybe you could also take a look at these other 
patches for uci?

https://patchwork.ozlabs.org/project/openwrt/patch/20221120010927.23856-2-...@venekamp.net/
https://patchwork.ozlabs.org/project/openwrt/patch/sy4p282mb39395204ae37bc45d35d834cc5...@sy4p282mb3939.ausp282.prod.outlook.com/

Kind regards,
Jan Venekamp


___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

[PATCH uci 2/2] remove internal usage of redundant uci_ptr.last

[Jan Venekamp]

In uci_lookup_ptr and uci_set the pointer uci_ptr ptr.last is set to
the element corresponding to the first of: ptr.o, ptr.s, ptr.p.

Thus, ptr.last is redundant and in case of uci_set is (and was) not
always consistently set.

In order to simplify the code this commit removes internal usage
of ptr.last, and remove setting it from uci_set (and from uci_add_list
that was never used anyway).

As it is part of the public C api ptr.last cannot be completely
removed though. A search on lxr.openwrt.org shows that it is used as
the output of uci_lookup_ptr in several packages.

So we leave setting ptr.last in uci_lookup_ptr intact.

Signed-off-by: Jan Venekamp 
---
 cli.c | 39 +++
 delta.c   | 10 ++
 list.c|  6 --
 lua/uci.c | 42 +++---
 4 files changed, 32 insertions(+), 65 deletions(-)

diff --git a/cli.c b/cli.c
index b4c4f95..f169e42 100644
--- a/cli.c
+++ b/cli.c
@@ -314,7 +314,6 @@ static void uci_show_changes(struct uci_package *p)
 
 static int package_cmd(int cmd, char *tuple)
 {
-   struct uci_element *e = NULL;
struct uci_ptr ptr;
int ret = 1;
 
@@ -323,7 +322,6 @@ static int package_cmd(int cmd, char *tuple)
return 1;
}
 
-   e = ptr.last;
switch(cmd) {
case CMD_CHANGES:
uci_show_changes(ptr.p);
@@ -349,20 +347,14 @@ static int package_cmd(int cmd, char *tuple)
cli_perror();
goto out;
}
-   switch(e->type) {
-   case UCI_TYPE_PACKAGE:
-   uci_show_package(ptr.p);
-   break;
-   case UCI_TYPE_SECTION:
-   uci_show_section(ptr.s);
-   break;
-   case UCI_TYPE_OPTION:
-   uci_show_option(ptr.o, true);
-   break;
-   default:
-   /* should not happen */
-   goto out;
-   }
+   if (ptr.o)
+   uci_show_option(ptr.o, true);
+   else if (ptr.s)
+   uci_show_section(ptr.s);
+   else if (ptr.p)
+   uci_show_package(ptr.p);
+   else
+   goto out; /* should not happen */
break;
}
 
@@ -475,7 +467,6 @@ done:
 
 static int uci_do_section_cmd(int cmd, int argc, char **argv)
 {
-   struct uci_element *e;
struct uci_ptr ptr;
int ret = UCI_OK;
int dummy;
@@ -493,7 +484,6 @@ static int uci_do_section_cmd(int cmd, int argc, char 
**argv)
(cmd != CMD_RENAME) && (cmd != CMD_REORDER))
return 1;
 
-   e = ptr.last;
switch(cmd) {
case CMD_GET:
if (!(ptr.flags & UCI_LOOKUP_COMPLETE)) {
@@ -501,17 +491,10 @@ static int uci_do_section_cmd(int cmd, int argc, char 
**argv)
cli_perror();
return 1;
}
-   switch(e->type) {
-   case UCI_TYPE_SECTION:
-   printf("%s\n", ptr.s->type);
-   break;
-   case UCI_TYPE_OPTION:
+   if (ptr.o)
uci_show_value(ptr.o, false);
-   break;
-   default:
-   break;
-   }
-   /* throw the value to stdout */
+   else if (ptr.s)
+   printf("%s\n", ptr.s->type);
break;
case CMD_RENAME:
ret = uci_rename(ctx, );
diff --git a/delta.c b/delta.c
index 85ec970..5437fc1 100644
--- a/delta.c
+++ b/delta.c
@@ -213,7 +213,6 @@ error:
 
 static void uci_parse_delta_line(struct uci_context *ctx, struct uci_package 
*p)
 {
-   struct uci_element *e = NULL;
struct uci_ptr ptr;
int cmd;
 
@@ -244,11 +243,14 @@ static void uci_parse_delta_line(struct uci_context *ctx, 
struct uci_package *p)
UCI_INTERNAL(uci_del_list, ctx, );
break;
case UCI_CMD_ADD:
+   UCI_INTERNAL(uci_set, ctx, );
+   if (!ptr.option && ptr.s)
+   ptr.s->anonymous = true;
+   break;
case UCI_CMD_CHANGE:
UCI_INTERNAL(uci_set, ctx, );
-   e = ptr.last;
-   if (!ptr.option && e && (cmd == UCI_CMD_ADD))
-   uci_to_section(e)->anonymous = true;
+   break;
+   default:
break;
}
return;
diff --git a/list.c b/list.c
index 1640213..304c9e1 100644
--- a/list.c
+++ b/list.c
@@ -616,7 +616,6 @@ int uci_add_list(struct uci_context *ctx, struct uci_pt

[PATCH uci 0/2] remove internal usage of redundant uci_ptr.last

[Jan Venekamp]

Remove internal usage of redundant uci_ptr.last in order to fix issue
and simplify code.

Jan Venekamp (2):
  file: Fix uci -m import command
  remove internal usage of redundant uci_ptr.last

 cli.c | 39 +++
 delta.c   | 10 ++
 file.c|  2 +-
 list.c|  6 --
 lua/uci.c | 42 +++---
 5 files changed, 33 insertions(+), 66 deletions(-)

-- 
2.32.0 (Apple Git-132)


___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

[PATCH uci 1/2] file: Fix uci -m import command

[Jan Venekamp]

Without this change we see the following error:
  # uci -m import optic < /etc/optic-db/default
  uci: Parse error (option/list command found before the first section) at line 
4, byte 1

ptr.last is still a null pointer in case the uci_lookup_list() call
found a matching section and set ptr.s to it. The code expects that
uci_set() updates the ptr.last pointer, but this is not done any more.
If case uci_lookup_list() did not found a section ptr->s is a null
pointer and then uci_set() will allocate a new section.

Fixes: ae61e1cad4a1 ("uci: optimize update section in uci_set")
Co-authored-by: Hauke Mehrtens 
Signed-off-by: Jan Venekamp 
---
 file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/file.c b/file.c
index 93abfae..6610f53 100644
--- a/file.c
+++ b/file.c
@@ -459,7 +459,7 @@ static void uci_parse_config(struct uci_context *ctx)
 
ctx->internal = !pctx->merge;
UCI_NESTED(uci_set, ctx, );
-   pctx->section = uci_to_section(ptr.last);
+   pctx->section = ptr.s;
}
 }
 
-- 
2.32.0 (Apple Git-132)


___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

[PATCH] netifd: Do not set disable_ipv6=0 in system_if_clear_state()

[Jan Venekamp]

When not configuring anything ipv6-related within uci and disabling
ipv6 by default for all interfaces with sysctl
net.ipv6.conf.all.disable_ipv6=1 some interfaces still get ipv6 turned
on due to system_set_disable_ipv6(dev, "0") at the end of
system_if_clear_state().

The line containing this call is a remnant of these two commits:
53f8ed2c 19/10/2011 [1] disable ipv6 for bridge member interfaces
98ca6746 07/04/2014 [2] netifd: Add interface config support to
enable/disable IPv6 in the kernel per device

In 53f8ed2c ipv6 is disabled for bridge members and enabled for all
ohter interfaces by adding calls to system_set_disable_ipv6() in
system_bridge_addif(), system_bridge_delif() and
system_if_clear_state().

In 98ca6746 the logic is reworked so that ipv6 can be optionally
configured per interface and ipv6 gets disabled by default for bridge
members in bridge_enable_member(). The calls to
system_set_disable_ipv6() are removed from system_bridge_addif() and
system_bridge_delif(). However, the line in system_if_clear_state() is
forgotten here and remains, having the effect that each interface will
have disable_ipv6=0 set.

Fix this by removing the remnant line.

[1] https://git.openwrt.org/?p=project/netifd.git;a=commitdiff;h=53f8ed2c
[2] https://git.openwrt.org/?p=project/netifd.git;a=commitdiff;h=98ca6746

Signed-off-by: Jan Venekamp 
---
 system-linux.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/system-linux.c b/system-linux.c
index 0760e73..4cfd95b 100644
--- a/system-linux.c
+++ b/system-linux.c
@@ -1353,7 +1353,6 @@ void system_if_clear_state(struct device *dev)
system_if_clear_entries(dev, RTM_GETADDR, AF_INET6);
system_if_clear_entries(dev, RTM_GETNEIGH, AF_INET);
system_if_clear_entries(dev, RTM_GETNEIGH, AF_INET6);
-   system_set_disable_ipv6(dev, "0");
 }
 
 static inline unsigned long
-- 
2.32.0 (Apple Git-132)


___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

[PATCH 1/1] uci: ignore wrong section / option name in uci_ptr

[Jan Venekamp]

Several functions that have a uci_ptr as argument expect if
ptr->s / ptr->o are provided that
strcmp(ptr->s->e.name, ptr->section) == 0 and
strcmp(ptr->o->e.name, ptr->option) == 0. Normally this is ensured
by a call to uci_lookup_ptr that precedes the function invocation and
by uci_expand_ptr.

However, when using the C api directly ptr->section and ptr->option
are not guarantied to be correct which can lead to unexpected or
inconsistent behaviour (breaking delta tracking).

Fix this by using ptr->section and ptr->option exclusively on new
section, new option and in the case of uci_revert. In other cases
use ptr->s->e.name and ptr->o->e.name exclusively.

Signed-off-by: Jan Venekamp 
---
 list.c | 61 --
 1 file changed, 25 insertions(+), 36 deletions(-)

diff --git a/list.c b/list.c
index 1640213..87b02ad 100644
--- a/list.c
+++ b/list.c
@@ -267,22 +267,6 @@ uci_free_package(struct uci_package **package)
*package = NULL;
 }
 
-static void
-uci_free_any(struct uci_element **e)
-{
-   switch((*e)->type) {
-   case UCI_TYPE_SECTION:
-   uci_free_section(uci_to_section(*e));
-   break;
-   case UCI_TYPE_OPTION:
-   uci_free_option(uci_to_option(*e));
-   break;
-   default:
-   break;
-   }
-   *e = NULL;
-}
-
 __private struct uci_element *
 uci_lookup_list(struct uci_list *list, const char *name)
 {
@@ -500,7 +484,7 @@ int uci_rename(struct uci_context *ctx, struct uci_ptr *ptr)
UCI_ASSERT(ctx, ptr->value);
 
if (!internal && p->has_delta)
-   uci_add_delta(ctx, >delta, UCI_CMD_RENAME, ptr->section, 
ptr->option, ptr->value);
+   uci_add_delta(ctx, >delta, UCI_CMD_RENAME, ptr->s->e.name, 
ptr->o ? ptr->o->e.name : NULL, ptr->value);
 
n = uci_strdup(ctx, ptr->value);
free(e->name);
@@ -552,25 +536,25 @@ int uci_delete(struct uci_context *ctx, struct uci_ptr 
*ptr)
/* NB: pass on internal flag to uci_del_element */
bool internal = ctx && ctx->internal;
struct uci_package *p;
-   struct uci_element *e1, *e2, *tmp;
+   struct uci_element *e, *tmp;
int index;
 
UCI_HANDLE_ERR(ctx);
 
-   e1 = uci_expand_ptr(ctx, ptr, true);
+   uci_expand_ptr(ctx, ptr, true);
p = ptr->p;
 
UCI_ASSERT(ctx, ptr->s);
 
-   if (ptr->o && ptr->o->type == UCI_TYPE_LIST && ptr->value && 
*ptr->value) {
+   if (ptr->o && ptr->o->type == UCI_TYPE_LIST && ptr->value && 
ptr->value[0]) {
if (!sscanf(ptr->value, "%d", ))
return 1;
 
-   uci_foreach_element_safe(>o->v.list, tmp, e2) {
+   uci_foreach_element_safe(>o->v.list, tmp, e) {
if (index == 0) {
if (!internal && p->has_delta)
-   uci_add_delta(ctx, >delta, 
UCI_CMD_REMOVE, ptr->section, ptr->option, ptr->value);
-   uci_free_option(uci_to_option(e2));
+   uci_add_delta(ctx, >delta, 
UCI_CMD_REMOVE, ptr->s->e.name, ptr->o->e.name, ptr->value);
+   uci_free_option(uci_to_option(e));
return 0;
}
index--;
@@ -580,14 +564,19 @@ int uci_delete(struct uci_context *ctx, struct uci_ptr 
*ptr)
}
 
if (!internal && p->has_delta)
-   uci_add_delta(ctx, >delta, UCI_CMD_REMOVE, ptr->section, 
ptr->option, NULL);
+   uci_add_delta(ctx, >delta, UCI_CMD_REMOVE, ptr->s->e.name, 
ptr->o ? ptr->o->e.name : NULL, NULL);
 
-   uci_free_any();
-
-   if (ptr->option)
+   if (ptr->o) {
+   if (ptr->option == ptr->o->e.name)
+   ptr->option = NULL;
+   uci_free_option(ptr->o);
ptr->o = NULL;
-   else if (ptr->section)
+   } else {
+   if (ptr->section == ptr->s->e.name)
+   ptr->section = NULL;
+   uci_free_section(ptr->s);
ptr->s = NULL;
+   }
 
return 0;
 }
@@ -622,7 +611,7 @@ int uci_add_list(struct uci_context *ctx, struct uci_ptr 
*ptr)
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, >e.list);
+   ptr->o = uci_alloc_list(ptr->s, ol

[PATCH 0/1] uci: ignore wrong section / option name in uci_ptr

[Jan Venekamp]

This patch depends on patch series:
[PATCH v2] uci: fixes for uci_set and uci_add_list

Jan Venekamp (1):
  uci: ignore wrong section / option name in uci_ptr

 list.c | 61 --
 1 file changed, 25 insertions(+), 36 deletions(-)

-- 
2.32.0 (Apple Git-132)


___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

[PATCH v2 7/9] uci: fix memory leak uci_set on update section

[Jan Venekamp]

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..3e8a87c 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(>options);
-   return;
-   }
-
-   /* fix pointers to HEAD at end/beginning of list */
-   uci_list_fixup(>options);
+   /* transfer the option list by inserting the new list HEAD and removing 
the old */
+   uci_list_insert(>options, >options);
+   uci_list_del(>options);
 
-   /* fix back pointer to section in options */
-   uci_foreach_element(>options, e) {
+   /* update pointer to section in options */
+   uci_foreach_element(>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(>sections, >e.list);
+   uci_list_insert(after ? after : p->sections.prev, >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 = >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 = >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 = >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(>s->options);
-   ptr->last = NULL;
- 

[PATCH v2 9/9] uci: macro uci_alloc_element not in uci.h

[Jan Venekamp]

The macro uci_alloc_element is in the public header file uci.h. However,
the macros output refers to uci_alloc_generic wich is in uci_internal.h
and not public. Thus, uci_alloc_element should be private as well and
moved to uci_internal.h.

Signed-off-by: Jan Venekamp 
---
 uci.h  | 10 --
 uci_internal.h |  3 +++
 2 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/uci.h b/uci.h
index b385e2b..d0374f2 100644
--- a/uci.h
+++ b/uci.h
@@ -613,16 +613,6 @@ BUILD_CAST(option)
 #define uci_to_option(ptr)  container_of(ptr, struct uci_option, e)
 #endif
 
-/**
- * uci_alloc_element: allocate a generic uci_element, reserve a buffer and 
typecast
- * @ctx: uci context
- * @type: {package,section,option}
- * @name: string containing the name of the element
- * @datasize: additional buffer size to reserve at the end of the struct
- */
-#define uci_alloc_element(ctx, type, name, datasize) \
-   uci_to_ ## type (uci_alloc_generic(ctx, uci_type_ ## type, name, 
sizeof(struct uci_ ## type) + datasize))
-
 #define uci_dataptr(ptr) \
(((char *) ptr) + sizeof(*ptr))
 
diff --git a/uci_internal.h b/uci_internal.h
index 34528f0..ff4ee8c 100644
--- a/uci_internal.h
+++ b/uci_internal.h
@@ -42,6 +42,9 @@ struct uci_parse_context
 #define pctx_char(pctx, i) ((pctx)->buf[(i)])
 #define pctx_cur_char(pctx)pctx_char(pctx, pctx_pos(pctx))
 
+#define uci_alloc_element(ctx, type, name, datasize) \
+   uci_to_ ## type (uci_alloc_generic(ctx, uci_type_ ## type, name, 
sizeof(struct uci_ ## type) + datasize))
+
 extern const char *uci_confdir;
 extern const char *uci_savedir;
 
-- 
2.32.0 (Apple Git-132)


___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

[PATCH v2 5/9] uci: fix atomicity of uci_add_list

[Jan Venekamp]

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, >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(>o->v.list, >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 = >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(>o->v.list, >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 = >o->e;
}
-   uci_add_element_list(ctx, ptr, internal);
+
+   /* add new item to list */
+   uci_list_add(>o->v.list, >list);
+
+   if (!internal && ptr->p->has_delta)
+   uci_add_delta(ctx, >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)
-- 
2.32.0 (Apple Git-132)


___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

[PATCH v2 0/9] uci: fixes for uci_set and uci_add_list

[Jan Venekamp]

When developing a C application that updates options with uci_set I
noticed that config files were not getting these updates. I found
that this was due to a use-after-free bug in uci_set.

Looking further at the code I also noticed some other issues. This
patch series contains fixes to these issues as well as some other
improvements.

Jan Venekamp (9):
  uci: fix use-after-free uci_set on update option
  uci: maintain option position in uci_set
  uci: optimize update option in uci_set
  uci: fix use-after-free uci_add_list
  uci: fix atomicity of uci_add_list
  uci: maintain option position in uci_add_list
  uci: fix memory leak uci_set on update section
  uci: optimize update section in uci_set
  uci: macro uci_alloc_element not in uci.h

 list.c| 168 --
 tests/shunit2/tests.d/090_cli_options |   8 +-
 uci.h |  10 --
 uci_internal.h|   3 +
 4 files changed, 87 insertions(+), 102 deletions(-)

Interdiff against v1:
diff --git a/list.c b/list.c
index 89c70f0..1640213 100644
--- a/list.c
+++ b/list.c
@@ -734,10 +734,10 @@ int uci_set(struct uci_context *ctx, struct uci_ptr *ptr)
strcpy(ptr->s->type, ptr->value);
} else {
struct uci_section *old = ptr->s;
-   ptr->s = uci_alloc_section(ptr->p, ptr->value, 
ptr->section, >e.list);
+   ptr->s = uci_alloc_section(ptr->p, ptr->value, 
old->e.name, >e.list);
uci_section_transfer_options(ptr->s, old);
if (ptr->section == old->e.name)
-   ptr->section = ptr->o->e.name;
+   ptr->section = ptr->s->e.name;
uci_free_section(old);
ptr->s->package->n_section--;
ptr->last = >s->e;
-- 
2.32.0 (Apple Git-132)


___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

[PATCH v2 8/9] uci: optimize update section in uci_set

[Jan Venekamp]

Optimize for the case when there is no need to update the section and
the case there is no need to reallocate memory when updating a section
in uci_set.

Signed-off-by: Jan Venekamp 
---
 list.c| 23 +++
 tests/shunit2/tests.d/090_cli_options |  8 +---
 2 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/list.c b/list.c
index 3e8a87c..1640213 100644
--- a/list.c
+++ b/list.c
@@ -727,14 +727,21 @@ int uci_set(struct uci_context *ctx, struct uci_ptr *ptr)
ptr->last = >o->e;
}
} else if (ptr->s && ptr->section) { /* update section */
-   struct uci_section *old = ptr->s;
-   ptr->s = uci_alloc_section(ptr->p, ptr->value, old->e.name, 
>e.list);
-   uci_section_transfer_options(ptr->s, old);
-   if (ptr->section == old->e.name)
-   ptr->section = ptr->s->e.name;
-   uci_free_section(old);
-   ptr->s->package->n_section--;
-   ptr->last = >s->e;
+   if (!strcmp(ptr->s->type, ptr->value))
+   return 0;
+
+   if (strlen(ptr->s->type) == strlen(ptr->value)) {
+   strcpy(ptr->s->type, ptr->value);
+   } else {
+   struct uci_section *old = ptr->s;
+   ptr->s = uci_alloc_section(ptr->p, ptr->value, 
old->e.name, >e.list);
+   uci_section_transfer_options(ptr->s, old);
+   if (ptr->section == old->e.name)
+   ptr->section = ptr->s->e.name;
+   uci_free_section(old);
+   ptr->s->package->n_section--;
+   ptr->last = >s->e;
+   }
} else {
UCI_THROW(ctx, UCI_ERR_INVAL);
}
diff --git a/tests/shunit2/tests.d/090_cli_options 
b/tests/shunit2/tests.d/090_cli_options
index 55920a2..aff8316 100644
--- a/tests/shunit2/tests.d/090_cli_options
+++ b/tests/shunit2/tests.d/090_cli_options
@@ -11,8 +11,9 @@ test_add_delta() {
# save new changes in "$new_savedir"
mkdir -p "$new_savedir"
touch "$new_savedir/delta"
-   $UCI -P "$new_savedir" set delta.sec0=sectype
+   $UCI -P "$new_savedir" set delta.sec0=tmptype
$UCI -P "$new_savedir" add_list delta.sec0.li0=1
+   $UCI -P "$new_savedir" set delta.sec0=sectype
 
assertEquals "delta.sec0='sectype'
 delta.sec0.li0+='0'" "$($UCI changes)"
@@ -22,8 +23,9 @@ delta.sec0.li0+='0'" "$($UCI changes)"
assertTrue "$?"
assertEquals "delta.sec0='sectype'
 delta.sec0.li0+='0'
-delta.sec0='sectype'
-delta.sec0.li0+='1'" "$cmdoutput"
+delta.sec0='tmptype'
+delta.sec0.li0+='1'
+delta.sec0='sectype'" "$cmdoutput"
 
# check combined export.  Order matters here.
cmdoutput="$($UCI -P "$new_savedir" export)"
-- 
2.32.0 (Apple Git-132)


___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

[PATCH v2 6/9] uci: maintain option position in uci_add_list

[Jan Venekamp]

Maintain the position of an option in the list when a string option
is converted to a list option in uci_add_list.

Signed-off-by: Jan Venekamp 
---
 list.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/list.c b/list.c
index 3518967..e6d631c 100644
--- a/list.c
+++ b/list.c
@@ -115,7 +115,7 @@ uci_free_option(struct uci_option *o)
 }
 
 static struct uci_option *
-uci_alloc_list(struct uci_section *s, const char *name)
+uci_alloc_list(struct uci_section *s, const char *name, struct uci_list *after)
 {
struct uci_package *p = s->package;
struct uci_context *ctx = p->ctx;
@@ -125,7 +125,7 @@ uci_alloc_list(struct uci_section *s, const char *name)
o->type = UCI_TYPE_LIST;
o->section = s;
uci_list_init(>v.list);
-   uci_list_add(>options, >e.list);
+   uci_list_insert(after ? after : s->options.prev, >e.list);
 
return o;
 }
@@ -628,7 +628,7 @@ int uci_add_list(struct uci_context *ctx, struct uci_ptr 
*ptr)
if (!ptr->o) {
/* create new list */
UCI_TRAP_SAVE(ctx, error);
-   ptr->o = uci_alloc_list(ptr->s, ptr->option);
+   ptr->o = uci_alloc_list(ptr->s, ptr->option, NULL);
UCI_TRAP_RESTORE(ctx);
ptr->last = >o->e;
} else if (ptr->o->type == UCI_TYPE_STRING) {
@@ -636,7 +636,7 @@ int uci_add_list(struct uci_context *ctx, struct uci_ptr 
*ptr)
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);
+   ptr->o = uci_alloc_list(ptr->s, ptr->option, >e.list);
UCI_TRAP_RESTORE(ctx);
uci_list_add(>o->v.list, >list);
 
-- 
2.32.0 (Apple Git-132)


___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

[PATCH v2 4/9] uci: fix use-after-free uci_add_list

[Jan Venekamp]

When uci_add_list is called with ptr->o set and ptr->option = NULL,
then in uci_expand_ptr ptr->option is set to ptr->o->e.name.
If ptr->o->type is UCI_TYPE_STRING then prev is set to ptr->o.
This will result in use-after-free because ptr->option is used in
the call to uci_add_delta in uci_add_element_list after
uci_free_option(prev).

Signed-off-by: Jan Venekamp 
---
 list.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/list.c b/list.c
index 5148dfd..ba099b6 100644
--- a/list.c
+++ b/list.c
@@ -652,6 +652,8 @@ int uci_add_list(struct uci_context *ctx, struct uci_ptr 
*ptr)
ptr->o = uci_alloc_list(ptr->s, ptr->option);
if (prev) {
uci_add_element_list(ctx, ptr, true);
+   if (ptr->option == prev->e.name)
+   ptr->option = ptr->o->e.name;
uci_free_option(prev);
ptr->value = value2;
}
-- 
2.32.0 (Apple Git-132)


___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

[PATCH v2 2/9] uci: maintain option position in uci_set

[Jan Venekamp]

Maintain the position of an option in the list when updating an option
in uci_set.

Signed-off-by: Jan Venekamp 
---
 list.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/list.c b/list.c
index ac3686c..a8f2a2c 100644
--- a/list.c
+++ b/list.c
@@ -76,7 +76,7 @@ uci_free_element(struct uci_element *e)
 }
 
 static struct uci_option *
-uci_alloc_option(struct uci_section *s, const char *name, const char *value)
+uci_alloc_option(struct uci_section *s, const char *name, const char *value, 
struct uci_list *after)
 {
struct uci_package *p = s->package;
struct uci_context *ctx = p->ctx;
@@ -87,7 +87,7 @@ uci_alloc_option(struct uci_section *s, const char *name, 
const char *value)
o->v.string = uci_dataptr(o);
o->section = s;
strcpy(o->v.string, value);
-   uci_list_add(>options, >e.list);
+   uci_list_insert(after ? after : s->options.prev, >e.list);
 
return o;
 }
@@ -719,7 +719,7 @@ int uci_set(struct uci_context *ctx, struct uci_ptr *ptr)
 
return uci_delete(ctx, ptr);
} else if (!ptr->o && ptr->option) { /* new option */
-   ptr->o = uci_alloc_option(ptr->s, ptr->option, ptr->value);
+   ptr->o = uci_alloc_option(ptr->s, ptr->option, ptr->value, 
NULL);
ptr->last = >o->e;
} else if (!ptr->s && ptr->section) { /* new section */
ptr->s = uci_alloc_section(ptr->p, ptr->value, ptr->section);
@@ -731,7 +731,7 @@ int uci_set(struct uci_context *ctx, struct uci_ptr *ptr)
!strcmp(ptr->o->v.string, ptr->value))
return 0;
 
-   ptr->o = uci_alloc_option(ptr->s, ptr->option, ptr->value);
+   ptr->o = uci_alloc_option(ptr->s, ptr->option, ptr->value, 
>e.list);
if (ptr->option == old->e.name)
ptr->option = ptr->o->e.name;
uci_free_option(old);
-- 
2.32.0 (Apple Git-132)


___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

[PATCH v2 1/9] uci: fix use-after-free uci_set on update option

[Jan Venekamp]

When uci_set is called with ptr->o set and ptr->option = NULL,
then in uci_expand_ptr ptr->option is set to ptr->o->e.name.
This will result in use-after-free because ptr->option is used in
the call to uci_add_delta after uci_free_option(ptr->o).

Signed-off-by: Jan Venekamp 
---
 list.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/list.c b/list.c
index 24ed2ee..ac3686c 100644
--- a/list.c
+++ b/list.c
@@ -725,15 +725,16 @@ int uci_set(struct uci_context *ctx, struct uci_ptr *ptr)
ptr->s = uci_alloc_section(ptr->p, ptr->value, ptr->section);
ptr->last = >s->e;
} else if (ptr->o && ptr->option) { /* update option */
-   struct uci_option *o;
+   struct uci_option *old = ptr->o;
 
if ((ptr->o->type == UCI_TYPE_STRING) &&
!strcmp(ptr->o->v.string, ptr->value))
return 0;
 
-   o = ptr->o;
ptr->o = uci_alloc_option(ptr->s, ptr->option, ptr->value);
-   uci_free_option(o);
+   if (ptr->option == old->e.name)
+   ptr->option = ptr->o->e.name;
+   uci_free_option(old);
ptr->last = >o->e;
} else if (ptr->s && ptr->section) { /* update section */
char *s = uci_strdup(ctx, ptr->value);
-- 
2.32.0 (Apple Git-132)


___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

[PATCH v2 3/9] uci: optimize update option in uci_set

[Jan Venekamp]

Optimize for the case when there is no need to reallocate memory when
updating an option in uci_set.

Signed-off-by: Jan Venekamp 
---
 list.c | 20 +++-
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/list.c b/list.c
index a8f2a2c..5148dfd 100644
--- a/list.c
+++ b/list.c
@@ -725,17 +725,19 @@ int uci_set(struct uci_context *ctx, struct uci_ptr *ptr)
ptr->s = uci_alloc_section(ptr->p, ptr->value, ptr->section);
ptr->last = >s->e;
} else if (ptr->o && ptr->option) { /* update option */
-   struct uci_option *old = ptr->o;
-
-   if ((ptr->o->type == UCI_TYPE_STRING) &&
-   !strcmp(ptr->o->v.string, ptr->value))
+   if (ptr->o->type == UCI_TYPE_STRING && 
!strcmp(ptr->o->v.string, ptr->value))
return 0;
 
-   ptr->o = uci_alloc_option(ptr->s, ptr->option, ptr->value, 
>e.list);
-   if (ptr->option == old->e.name)
-   ptr->option = ptr->o->e.name;
-   uci_free_option(old);
-   ptr->last = >o->e;
+   if (ptr->o->type == UCI_TYPE_STRING && strlen(ptr->o->v.string) 
== strlen(ptr->value)) {
+   strcpy(ptr->o->v.string, ptr->value);
+   } else {
+   struct uci_option *old = ptr->o;
+   ptr->o = uci_alloc_option(ptr->s, ptr->option, 
ptr->value, >e.list);
+   if (ptr->option == old->e.name)
+   ptr->option = ptr->o->e.name;
+   uci_free_option(old);
+   ptr->last = >o->e;
+   }
} else if (ptr->s && ptr->section) { /* update section */
char *s = uci_strdup(ctx, ptr->value);
 
-- 
2.32.0 (Apple Git-132)


___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

[PATCH 9/9] uci: macro uci_alloc_element not in uci.h

[Jan Venekamp]

The macro uci_alloc_element is in the public header file uci.h. However,
the macros output refers to uci_alloc_generic wich is in uci_internal.h
and not public. Thus, uci_alloc_element should be private as well and
moved to uci_internal.h.

Signed-off-by: Jan Venekamp 
---
 uci.h  | 10 --
 uci_internal.h |  3 +++
 2 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/uci.h b/uci.h
index b385e2b..d0374f2 100644
--- a/uci.h
+++ b/uci.h
@@ -613,16 +613,6 @@ BUILD_CAST(option)
 #define uci_to_option(ptr)  container_of(ptr, struct uci_option, e)
 #endif
 
-/**
- * uci_alloc_element: allocate a generic uci_element, reserve a buffer and 
typecast
- * @ctx: uci context
- * @type: {package,section,option}
- * @name: string containing the name of the element
- * @datasize: additional buffer size to reserve at the end of the struct
- */
-#define uci_alloc_element(ctx, type, name, datasize) \
-   uci_to_ ## type (uci_alloc_generic(ctx, uci_type_ ## type, name, 
sizeof(struct uci_ ## type) + datasize))
-
 #define uci_dataptr(ptr) \
(((char *) ptr) + sizeof(*ptr))
 
diff --git a/uci_internal.h b/uci_internal.h
index 34528f0..ff4ee8c 100644
--- a/uci_internal.h
+++ b/uci_internal.h
@@ -42,6 +42,9 @@ struct uci_parse_context
 #define pctx_char(pctx, i) ((pctx)->buf[(i)])
 #define pctx_cur_char(pctx)pctx_char(pctx, pctx_pos(pctx))
 
+#define uci_alloc_element(ctx, type, name, datasize) \
+   uci_to_ ## type (uci_alloc_generic(ctx, uci_type_ ## type, name, 
sizeof(struct uci_ ## type) + datasize))
+
 extern const char *uci_confdir;
 extern const char *uci_savedir;
 
-- 
2.32.0 (Apple Git-132)


___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

[PATCH 8/9] uci: optimize update section in uci_set

[Jan Venekamp]

Optimize for the case when there is no need to update the section and
the case there is no need to reallocate memory when updating a section
in uci_set.

Signed-off-by: Jan Venekamp 
---
 list.c| 23 +++
 tests/shunit2/tests.d/090_cli_options |  8 +---
 2 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/list.c b/list.c
index 783836b..89c70f0 100644
--- a/list.c
+++ b/list.c
@@ -727,14 +727,21 @@ int uci_set(struct uci_context *ctx, struct uci_ptr *ptr)
ptr->last = >o->e;
}
} else if (ptr->s && ptr->section) { /* update section */
-   struct uci_section *old = ptr->s;
-   ptr->s = uci_alloc_section(ptr->p, ptr->value, ptr->section, 
>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 = >s->e;
+   if (!strcmp(ptr->s->type, ptr->value))
+   return 0;
+
+   if (strlen(ptr->s->type) == strlen(ptr->value)) {
+   strcpy(ptr->s->type, ptr->value);
+   } else {
+   struct uci_section *old = ptr->s;
+   ptr->s = uci_alloc_section(ptr->p, ptr->value, 
ptr->section, >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 = >s->e;
+   }
} else {
UCI_THROW(ctx, UCI_ERR_INVAL);
}
diff --git a/tests/shunit2/tests.d/090_cli_options 
b/tests/shunit2/tests.d/090_cli_options
index 55920a2..aff8316 100644
--- a/tests/shunit2/tests.d/090_cli_options
+++ b/tests/shunit2/tests.d/090_cli_options
@@ -11,8 +11,9 @@ test_add_delta() {
# save new changes in "$new_savedir"
mkdir -p "$new_savedir"
touch "$new_savedir/delta"
-   $UCI -P "$new_savedir" set delta.sec0=sectype
+   $UCI -P "$new_savedir" set delta.sec0=tmptype
$UCI -P "$new_savedir" add_list delta.sec0.li0=1
+   $UCI -P "$new_savedir" set delta.sec0=sectype
 
assertEquals "delta.sec0='sectype'
 delta.sec0.li0+='0'" "$($UCI changes)"
@@ -22,8 +23,9 @@ delta.sec0.li0+='0'" "$($UCI changes)"
assertTrue "$?"
assertEquals "delta.sec0='sectype'
 delta.sec0.li0+='0'
-delta.sec0='sectype'
-delta.sec0.li0+='1'" "$cmdoutput"
+delta.sec0='tmptype'
+delta.sec0.li0+='1'
+delta.sec0='sectype'" "$cmdoutput"
 
# check combined export.  Order matters here.
cmdoutput="$($UCI -P "$new_savedir" export)"
-- 
2.32.0 (Apple Git-132)


___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

[PATCH 7/9] uci: fix memory leak uci_set on update section

[Jan Venekamp]

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(>options);
-   return;
-   }
-
-   /* fix pointers to HEAD at end/beginning of list */
-   uci_list_fixup(>options);
+   /* transfer the option list by inserting the new list HEAD and removing 
the old */
+   uci_list_insert(>options, >options);
+   uci_list_del(>options);
 
-   /* fix back pointer to section in options */
-   uci_foreach_element(>options, e) {
+   /* update pointer to section in options */
+   uci_foreach_element(>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(>sections, >e.list);
+   uci_list_insert(after ? after : p->sections.prev, >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 = >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 = >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 = >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(>s->options);
-   ptr->last = NULL;
- 

[PATCH 5/9] uci: fix atomicity of uci_add_list

[Jan Venekamp]

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, >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(>o->v.list, >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 = >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(>o->v.list, >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 = >o->e;
}
-   uci_add_element_list(ctx, ptr, internal);
+
+   /* add new item to list */
+   uci_list_add(>o->v.list, >list);
+
+   if (!internal && ptr->p->has_delta)
+   uci_add_delta(ctx, >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)
-- 
2.32.0 (Apple Git-132)


___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

[PATCH 6/9] uci: maintain option position in uci_add_list

[Jan Venekamp]

Maintain the position of an option in the list when a string option
is converted to a list option in uci_add_list.

Signed-off-by: Jan Venekamp 
---
 list.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/list.c b/list.c
index 3518967..e6d631c 100644
--- a/list.c
+++ b/list.c
@@ -115,7 +115,7 @@ uci_free_option(struct uci_option *o)
 }
 
 static struct uci_option *
-uci_alloc_list(struct uci_section *s, const char *name)
+uci_alloc_list(struct uci_section *s, const char *name, struct uci_list *after)
 {
struct uci_package *p = s->package;
struct uci_context *ctx = p->ctx;
@@ -125,7 +125,7 @@ uci_alloc_list(struct uci_section *s, const char *name)
o->type = UCI_TYPE_LIST;
o->section = s;
uci_list_init(>v.list);
-   uci_list_add(>options, >e.list);
+   uci_list_insert(after ? after : s->options.prev, >e.list);
 
return o;
 }
@@ -628,7 +628,7 @@ int uci_add_list(struct uci_context *ctx, struct uci_ptr 
*ptr)
if (!ptr->o) {
/* create new list */
UCI_TRAP_SAVE(ctx, error);
-   ptr->o = uci_alloc_list(ptr->s, ptr->option);
+   ptr->o = uci_alloc_list(ptr->s, ptr->option, NULL);
UCI_TRAP_RESTORE(ctx);
ptr->last = >o->e;
} else if (ptr->o->type == UCI_TYPE_STRING) {
@@ -636,7 +636,7 @@ int uci_add_list(struct uci_context *ctx, struct uci_ptr 
*ptr)
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);
+   ptr->o = uci_alloc_list(ptr->s, ptr->option, >e.list);
UCI_TRAP_RESTORE(ctx);
uci_list_add(>o->v.list, >list);
 
-- 
2.32.0 (Apple Git-132)


___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

[PATCH 2/9] uci: maintain option position in uci_set

[Jan Venekamp]

Maintain the position of an option in the list when updating an option
in uci_set.

Signed-off-by: Jan Venekamp 
---
 list.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/list.c b/list.c
index ac3686c..a8f2a2c 100644
--- a/list.c
+++ b/list.c
@@ -76,7 +76,7 @@ uci_free_element(struct uci_element *e)
 }
 
 static struct uci_option *
-uci_alloc_option(struct uci_section *s, const char *name, const char *value)
+uci_alloc_option(struct uci_section *s, const char *name, const char *value, 
struct uci_list *after)
 {
struct uci_package *p = s->package;
struct uci_context *ctx = p->ctx;
@@ -87,7 +87,7 @@ uci_alloc_option(struct uci_section *s, const char *name, 
const char *value)
o->v.string = uci_dataptr(o);
o->section = s;
strcpy(o->v.string, value);
-   uci_list_add(>options, >e.list);
+   uci_list_insert(after ? after : s->options.prev, >e.list);
 
return o;
 }
@@ -719,7 +719,7 @@ int uci_set(struct uci_context *ctx, struct uci_ptr *ptr)
 
return uci_delete(ctx, ptr);
} else if (!ptr->o && ptr->option) { /* new option */
-   ptr->o = uci_alloc_option(ptr->s, ptr->option, ptr->value);
+   ptr->o = uci_alloc_option(ptr->s, ptr->option, ptr->value, 
NULL);
ptr->last = >o->e;
} else if (!ptr->s && ptr->section) { /* new section */
ptr->s = uci_alloc_section(ptr->p, ptr->value, ptr->section);
@@ -731,7 +731,7 @@ int uci_set(struct uci_context *ctx, struct uci_ptr *ptr)
!strcmp(ptr->o->v.string, ptr->value))
return 0;
 
-   ptr->o = uci_alloc_option(ptr->s, ptr->option, ptr->value);
+   ptr->o = uci_alloc_option(ptr->s, ptr->option, ptr->value, 
>e.list);
if (ptr->option == old->e.name)
ptr->option = ptr->o->e.name;
uci_free_option(old);
-- 
2.32.0 (Apple Git-132)


___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

[PATCH 4/9] uci: fix use-after-free uci_add_list

[Jan Venekamp]

When uci_add_list is called with ptr->o set and ptr->option = NULL,
then in uci_expand_ptr ptr->option is set to ptr->o->e.name.
If ptr->o->type is UCI_TYPE_STRING then prev is set to ptr->o.
This will result in use-after-free because ptr->option is used in
the call to uci_add_delta in uci_add_element_list after
uci_free_option(prev).

Signed-off-by: Jan Venekamp 
---
 list.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/list.c b/list.c
index 5148dfd..ba099b6 100644
--- a/list.c
+++ b/list.c
@@ -652,6 +652,8 @@ int uci_add_list(struct uci_context *ctx, struct uci_ptr 
*ptr)
ptr->o = uci_alloc_list(ptr->s, ptr->option);
if (prev) {
uci_add_element_list(ctx, ptr, true);
+   if (ptr->option == prev->e.name)
+   ptr->option = ptr->o->e.name;
uci_free_option(prev);
ptr->value = value2;
}
-- 
2.32.0 (Apple Git-132)


___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

[PATCH 3/9] uci: optimize update option in uci_set

[Jan Venekamp]

Optimize for the case when there is no need to reallocate memory when
updating an option in uci_set.

Signed-off-by: Jan Venekamp 
---
 list.c | 20 +++-
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/list.c b/list.c
index a8f2a2c..5148dfd 100644
--- a/list.c
+++ b/list.c
@@ -725,17 +725,19 @@ int uci_set(struct uci_context *ctx, struct uci_ptr *ptr)
ptr->s = uci_alloc_section(ptr->p, ptr->value, ptr->section);
ptr->last = >s->e;
} else if (ptr->o && ptr->option) { /* update option */
-   struct uci_option *old = ptr->o;
-
-   if ((ptr->o->type == UCI_TYPE_STRING) &&
-   !strcmp(ptr->o->v.string, ptr->value))
+   if (ptr->o->type == UCI_TYPE_STRING && 
!strcmp(ptr->o->v.string, ptr->value))
return 0;
 
-   ptr->o = uci_alloc_option(ptr->s, ptr->option, ptr->value, 
>e.list);
-   if (ptr->option == old->e.name)
-   ptr->option = ptr->o->e.name;
-   uci_free_option(old);
-   ptr->last = >o->e;
+   if (ptr->o->type == UCI_TYPE_STRING && strlen(ptr->o->v.string) 
== strlen(ptr->value)) {
+   strcpy(ptr->o->v.string, ptr->value);
+   } else {
+   struct uci_option *old = ptr->o;
+   ptr->o = uci_alloc_option(ptr->s, ptr->option, 
ptr->value, >e.list);
+   if (ptr->option == old->e.name)
+   ptr->option = ptr->o->e.name;
+   uci_free_option(old);
+   ptr->last = >o->e;
+   }
} else if (ptr->s && ptr->section) { /* update section */
char *s = uci_strdup(ctx, ptr->value);
 
-- 
2.32.0 (Apple Git-132)


___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

[PATCH 1/9] uci: fix use-after-free uci_set on update option

[Jan Venekamp]

When uci_set is called with ptr->o set and ptr->option = NULL,
then in uci_expand_ptr ptr->option is set to ptr->o->e.name.
This will result in use-after-free because ptr->option is used in
the call to uci_add_delta after uci_free_option(ptr->o).

Signed-off-by: Jan Venekamp 
---
 list.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/list.c b/list.c
index 24ed2ee..ac3686c 100644
--- a/list.c
+++ b/list.c
@@ -725,15 +725,16 @@ int uci_set(struct uci_context *ctx, struct uci_ptr *ptr)
ptr->s = uci_alloc_section(ptr->p, ptr->value, ptr->section);
ptr->last = >s->e;
} else if (ptr->o && ptr->option) { /* update option */
-   struct uci_option *o;
+   struct uci_option *old = ptr->o;
 
if ((ptr->o->type == UCI_TYPE_STRING) &&
!strcmp(ptr->o->v.string, ptr->value))
return 0;
 
-   o = ptr->o;
ptr->o = uci_alloc_option(ptr->s, ptr->option, ptr->value);
-   uci_free_option(o);
+   if (ptr->option == old->e.name)
+   ptr->option = ptr->o->e.name;
+   uci_free_option(old);
ptr->last = >o->e;
} else if (ptr->s && ptr->section) { /* update section */
char *s = uci_strdup(ctx, ptr->value);
-- 
2.32.0 (Apple Git-132)


___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel

[PATCH 0/9] uci: fixes for uci_set and uci_add_list

[Jan Venekamp]

When developing a C application that updates options with uci_set I
noticed that config files were not getting these updates. I found
that this was due to a use-after-free bug in uci_set.

Looking further at the code I also noticed some other issues. This
patch series contains fixes to these issues as well as some other
improvements.

Jan Venekamp (9):
  uci: fix use-after-free uci_set on update option
  uci: maintain option position in uci_set
  uci: optimize update option in uci_set
  uci: fix use-after-free uci_add_list
  uci: fix atomicity of uci_add_list
  uci: maintain option position in uci_add_list
  uci: fix memory leak uci_set on update section
  uci: optimize update section in uci_set
  uci: macro uci_alloc_element not in uci.h

 list.c| 168 --
 tests/shunit2/tests.d/090_cli_options |   8 +-
 uci.h |  10 --
 uci_internal.h|   3 +
 4 files changed, 87 insertions(+), 102 deletions(-)

-- 
2.32.0 (Apple Git-132)


___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel