Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FS#1321 - UCI potential invalid memory access when updating existing section #5783

Open
openwrt-bot opened this issue Feb 2, 2018 · 3 comments
Open
Labels
flyspray packages

Comments

@openwrt-bot
Copy link

@openwrt-bot openwrt-bot commented Feb 2, 2018

mondwan:

  • Software versions

[[
https://git.openwrt.org/?p=project/uci.git;a=tree;h=5beb95da3dbec6db11a6bdfaab7807ee2daf41e6;hb=5beb95da3dbec6db11a6bdfaab7807ee2daf41e6|UCI 2018-01-01]]

  • Device problem occurs on

There is a potential memory leak when updating existing section in [[
https://git.openwrt.org/?p=project/uci.git;a=blob;f=list.c;h=0347138cbbc19bc9a5ad5c6231ee528f2492aab0;hb=5beb95da3dbec6db11a6bdfaab7807ee2daf41e6#l709|list.c:709]]

Return pointer from realloc may not be the same as ptr->s. Due to realloc mechanism, pointers value from ptr->s->options are copied to the ptr->last. However, those pointers (ptr->last->s->options) are pointing back to the ptr->s which has been freed.

Below are steps to reproduce.

  • Steps to reproduce

Given a config file like that

cat /etc/config/network config interface wan

Given test codes like that

#include

int main(int argc, const char *argv[])
{
struct uci_context *uci_ctx = uci_alloc_context();
struct uci_ptr ptr = {0};
ptr.package = "network";
ptr.section = "wan";
ptr.value = "interface";
uci_lookup_ptr(uci_ctx, &ptr, NULL, false);
uci_set(uci_ctx, &ptr);
uci_save(uci_ctx, ptr.p);
uci_commit(uci_ctx, &ptr.p, false);
uci_free_context(uci_ctx);

return 0;

}

Runs like that

cd UCI_PROJECT_ROOT mkdir build cmake .. make valgrind ./test/test_mem_leak_section .... CTRL-C after a while ....

Here is the output of valgrind before the hotfix below

==1567== Memcheck, a memory error detector
==1567== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==1567== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==1567== Command: ./test/test_mem_leak_section
==1567==
==1567== Invalid write of size 8
==1567== at 0x4E3946B: uci_list_del (uci_internal.h:116)
==1567== by 0x4E3946B: uci_free_element (list.c:71)
==1567== by 0x4E394F4: uci_free_section (list.c:211)
==1567== by 0x4E39590: uci_free_package (list.c:243)
==1567== by 0x4E39C97: uci_free_context (libuci.c:84)
==1567== by 0x400719: main (test_mem_leak_section.c:12)
==1567== Address 0x561a850 is 32 bytes inside a block of size 82 free'd
==1567== at 0x4C2CE8E: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1567== by 0x4E3C4FE: uci_realloc (util.c:49)
==1567== by 0x4E3AC77: uci_set (list.c:709)
==1567== by 0x400711: main (test_mem_leak_section.c:11)
==1567==
==1567== Invalid write of size 8
==1567== at 0x4E3946E: uci_list_del (uci_internal.h:117)
==1567== by 0x4E3946E: uci_free_element (list.c:71)
==1567== by 0x4E394F4: uci_free_section (list.c:211)
==1567== by 0x4E39590: uci_free_package (list.c:243)
==1567== by 0x4E39C97: uci_free_context (libuci.c:84)
==1567== by 0x400719: main (test_mem_leak_section.c:12)
==1567== Address 0x561a858 is 40 bytes inside a block of size 82 free'd
==1567== at 0x4C2CE8E: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1567== by 0x4E3C4FE: uci_realloc (util.c:49)
==1567== by 0x4E3AC77: uci_set (list.c:709)
==1567== by 0x400711: main (test_mem_leak_section.c:11)
==1567==
==1567== Invalid read of size 8
==1567== at 0x4E394F8: uci_free_section (list.c:210)
==1567== by 0x4E39590: uci_free_package (list.c:243)
==1567== by 0x4E39C97: uci_free_context (libuci.c:84)
==1567== by 0x400719: main (test_mem_leak_section.c:12)
==1567== Address 0x561a850 is 32 bytes inside a block of size 82 free'd
==1567== at 0x4C2CE8E: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1567== by 0x4E3C4FE: uci_realloc (util.c:49)
==1567== by 0x4E3AC77: uci_set (list.c:709)
==1567== by 0x400711: main (test_mem_leak_section.c:11)
==1567==
==1567== Invalid read of size 4
==1567== at 0x4E39486: uci_free_option (list.c:97)
==1567== by 0x4E394F4: uci_free_section (list.c:211)
==1567== by 0x4E39590: uci_free_package (list.c:243)
==1567== by 0x4E39C97: uci_free_context (libuci.c:84)
==1567== by 0x400719: main (test_mem_leak_section.c:12)
==1567== Address 0x561a878 is 72 bytes inside a block of size 82 free'd
==1567== at 0x4C2CE8E: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1567== by 0x4E3C4FE: uci_realloc (util.c:49)
==1567== by 0x4E3AC77: uci_set (list.c:709)
==1567== by 0x400711: main (test_mem_leak_section.c:11)
==1567==
==1567== Invalid read of size 8
==1567== at 0x4E39456: uci_free_element (list.c:69)
==1567== by 0x4E394F4: uci_free_section (list.c:211)
==1567== by 0x4E39590: uci_free_package (list.c:243)
==1567== by 0x4E39C97: uci_free_context (libuci.c:84)
==1567== by 0x400719: main (test_mem_leak_section.c:12)
==1567== Address 0x561a868 is 56 bytes inside a block of size 82 free'd
==1567== at 0x4C2CE8E: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1567== by 0x4E3C4FE: uci_realloc (util.c:49)
==1567== by 0x4E3AC77: uci_set (list.c:709)
==1567== by 0x400711: main (test_mem_leak_section.c:11)
==1567==
==1567== Invalid read of size 8
==1567== at 0x4E3945F: uci_free_element (list.c:70)
==1567== by 0x4E394F4: uci_free_section (list.c:211)
==1567== by 0x4E39590: uci_free_package (list.c:243)
==1567== by 0x4E39C97: uci_free_context (libuci.c:84)
==1567== by 0x400719: main (test_mem_leak_section.c:12)
==1567== Address 0x561a850 is 32 bytes inside a block of size 82 free'd
==1567== at 0x4C2CE8E: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1567== by 0x4E3C4FE: uci_realloc (util.c:49)
==1567== by 0x4E3AC77: uci_set (list.c:709)
==1567== by 0x400711: main (test_mem_leak_section.c:11)
==1567==
==1567== Invalid free() / delete / delete[] / realloc()
==1567== at 0x4C2BDEC: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1567== by 0x4E394F4: uci_free_section (list.c:211)
==1567== by 0x4E39590: uci_free_package (list.c:243)
==1567== by 0x4E39C97: uci_free_context (libuci.c:84)
==1567== by 0x400719: main (test_mem_leak_section.c:12)
==1567== Address 0x561a850 is 32 bytes inside a block of size 82 free'd
==1567== at 0x4C2CE8E: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1567== by 0x4E3C4FE: uci_realloc (util.c:49)
==1567== by 0x4E3AC77: uci_set (list.c:709)
==1567== by 0x400711: main (test_mem_leak_section.c:11)
==1567==
^C
==1567==
==1567== HEAP SUMMARY:
==1567== in use at exit: 973 bytes in 17 blocks
==1567== total heap usage: 189 allocs, 408,927 frees, 8,483 bytes allocated
==1567==
==1567== LEAK SUMMARY:
==1567== definitely lost: 0 bytes in 0 blocks
==1567== indirectly lost: 0 bytes in 0 blocks
==1567== possibly lost: 0 bytes in 0 blocks
==1567== still reachable: 973 bytes in 17 blocks
==1567== suppressed: 0 bytes in 0 blocks
==1567== Rerun with --leak-check=full to see details of leaked memory
==1567==
==1567== For counts of detected and suppressed errors, rerun with: -v
==1567== ERROR SUMMARY: 2043783 errors from 7 contexts (suppressed: 0 from 0)

  • Hot fix
- 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); + ptr->last = (struct uci_element *) ptr->s;

Please take a look the attachment. It includes my hot fix for this issue and corresponding demo codes as illustrated above.

I am not sure how to "update" the "uci_options" associated to the "uci_section". So, I simply omit and replace the realloc part.

Tested by valgrind again

valgrind ./test/test_mem_leak_section ==1749== Memcheck, a memory error detector ==1749== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al. ==1749== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info ==1749== Command: ./test/test_mem_leak_section ==1749== ==1749== ==1749== HEAP SUMMARY: ==1749== in use at exit: 0 bytes in 0 blocks ==1749== total heap usage: 392 allocs, 392 frees, 23,574 bytes allocated ==1749== ==1749== All heap blocks were freed -- no leaks are possible ==1749== ==1749== For counts of detected and suppressed errors, rerun with: -v ==1749== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
@openwrt-bot
Copy link
Author

@openwrt-bot openwrt-bot commented Oct 10, 2018

mondwan:

Hello, may I know why my patch does not accepted? Do I made something wrong for this? Is there anything I can help on this issue?

@openwrt-bot
Copy link
Author

@openwrt-bot openwrt-bot commented Oct 10, 2018

dedeckeh:

Hi,

Only patches either submitted via github PRs or patchwork are reviewed and applied to the source tree. In this particular case you need to send the uci patch to to the openwrt-devel mailing list; see also https://openwrt.org/submitting-patches

@openwrt-bot
Copy link
Author

@openwrt-bot openwrt-bot commented Nov 21, 2018

mondwan:

Sorry for replying late.

It seems like there is no github repository for project UCI. Does it means that I must submit the patch via patchwork? As I am not familiar with the mailing way, I would prefer to submit a PR via Github. What should I do?

Thanks for your time

@aparcar aparcar added the packages label Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flyspray packages
Projects
None yet
Development

No branches or pull requests

2 participants