| Message ID | 20230829115846.1231689-1-waherob42858@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [uci] uci: decrease the n_section when section is freed | expand |
No idea whether this fix is entirely correct but I also encountered the issue and the patch fixes them. When a large enough number of sections is deleted, it is also possible to go out of bounds leading to segmentation faults. On Tue, 29 Aug 2023 19:58:46 +0800 Jeff Shiu <waherob42858@gmail.com> wrote: > The package n_section counter increases when a section is allocated > but does not decrease when a section is freed. > > 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.
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 0000000..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 <<EOF +add batch_anonymous_section foo +set batch_anonymous_section.@foo[-1].bar='0' +add batch_anonymous_section foo +set batch_anonymous_section.@foo[-1].bar='1' +del batch_anonymous_section.@foo[0] +add batch_anonymous_section foo +set batch_anonymous_section.@foo[-1].bar='2' +EOF + ${UCI} commit + assertSameFile "${REF_DIR}/batch_anonymous_section.result" "${CONFIG_DIR}/batch_anonymous_section" +}
Hi Jan,
On Thu, 7 Sep 2023 18:12:56 +0200
Jan Venekamp <jan@venekamp.net> wrote:
> Could you provided a example / test case for this behaviour?
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.
It was set up like that because, at the time, I thought the problem had to do with packages.
If enough sections were created and deleted, I would reliably segfault when iterating sections (e.g. uci_foreach_element_safe(&u_ptr.p->sections, tmp, e)).
That is how I discovered the bug.
Here's the output I had saved for a submission to this mailing list:
[root@wpj428 ~] $ /tmp/delta-test 20 # <- the amount of sections to be created and deleted
has delta: dhcp
has delta: dropbear
has delta: fastd
has delta: fstab
has delta: network
has delta: radius
has delta: system
has delta: ubootenv
has delta: wireless
[root@wpj428 ~] $ uci changes | grep fastd
[root@wpj428 ~] $ uci changes ubootenv
[root@wpj428 ~] $
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!
kind regards,
Leon
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(&u_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.
diff --git a/list.c b/list.c index 304c9e1..6545ba5 100644 --- a/list.c +++ b/list.c @@ -228,6 +228,7 @@ uci_free_section(struct uci_section *s) if ((s->type != uci_dataptr(s)) && (s->type != NULL)) free(s->type); + s->package->n_section--; uci_free_element(&s->e); } @@ -734,7 +735,6 @@ int uci_set(struct uci_context *ctx, struct uci_ptr *ptr) if (ptr->section == old->e.name) ptr->section = ptr->s->e.name; uci_free_section(old); - ptr->s->package->n_section--; } } else { UCI_THROW(ctx, UCI_ERR_INVAL);
The package n_section counter increases when a section is allocated but does not decrease when a section is freed. 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. Signed-off-by: Jeff Shiu <waherob42858@gmail.com> --- list.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)