OpenWrt/LEDE Project

  • Status Unconfirmed
  • Percent Complete
    0%
  • Task Type Bug Report
  • Category Packages
  • Assigned To No-one
  • Operating System All
  • Severity Low
  • Priority Very Low
  • Reported Version Trunk
  • Due in Version Undecided
  • Due Date Undecided
  • Private
Attached to Project: OpenWrt/LEDE Project
Opened by Mond WAN - 02.02.2018

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

- 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)


Mond WAN commented on 10.10.2018 02:54

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?

Project Manager
Hans Dedecker commented on 10.10.2018 07:21

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

Mond WAN commented on 21.11.2018 03:04

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

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing