UCI potential invalid memory access when updating existing section
Username: Mond WAN
Origin: https://bugs.openwrt.org/index.php?do=details&task_id=1321
- Software versions
UCI 2018-01-01
- Device problem occurs on
There is a potential memory leak when updating existing section in
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 <uci.h>
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)