| Message ID | 20180812063533.29085-1-mrkiko.rs@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | John Crispin |
| Headers | show |
| Series | [OpenWrt-Devel] uci: do not access invalid memory when updating an existing section | expand |
On Sun, 12 Aug 2018 at 14:41, Enrico Mioso <mrkiko.rs@gmail.com> wrote: > > If a new section with the same name and type of an old one is found, a > memory reallocation happens. Still, the options list for the section is > not reinitialized, hence a stale pointer is being used. > > Signed-off-by: Enrico Mioso <mrkiko.rs@gmail.com> > --- > list.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/list.c b/list.c > index 25aec56..d934216 100644 > --- a/list.c > +++ b/list.c > @@ -717,6 +717,7 @@ int uci_set(struct uci_context *ctx, struct uci_ptr *ptr) > 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); > + uci_list_init(&ptr->s->options); > } else { > free(ptr->s->type); > } If I understand the original code right, the intention is to just update the section type as long as the name matches, regardless of whether there is a section type change. In both cases, there will be cleanup of existing options list. This is an established behavior and should not be changed for backward compatibility considerations. Regards, yousong
Hello, and thank you for your help, and review. I agree on not changing behaviour. the problem is not actually a memory leak, but the fact that ptr->s->options seems to not be a valid pointer at that point. At least, this is what valgrind suggested, if I am not wrong or interpreting wrongly the output. Thanks again for all, Enrico On Mon, Aug 13, 2018 at 09:07:32PM +0800, Yousong Zhou wrote: > On Sun, 12 Aug 2018 at 14:41, Enrico Mioso <mrkiko.rs@gmail.com> wrote: > > > > If a new section with the same name and type of an old one is found, a > > memory reallocation happens. Still, the options list for the section is > > not reinitialized, hence a stale pointer is being used. > > > > Signed-off-by: Enrico Mioso <mrkiko.rs@gmail.com> > > --- > > list.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/list.c b/list.c > > index 25aec56..d934216 100644 > > --- a/list.c > > +++ b/list.c > > @@ -717,6 +717,7 @@ int uci_set(struct uci_context *ctx, struct uci_ptr *ptr) > > 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); > > + uci_list_init(&ptr->s->options); > > } else { > > free(ptr->s->type); > > } > > If I understand the original code right, the intention is to just > update the section type as long as the name matches, regardless of > whether there is a section type change. In both cases, there will be > cleanup of existing options list. This is an established behavior and > should not be changed for backward compatibility considerations. > > Regards, > yousong
On Tue, 14 Aug 2018 at 04:56, Enrico Mioso <mrkiko.rs@gmail.com> wrote: > > Hello, and thank you for your help, and review. > I agree on not changing behaviour. the problem is not actually a memory leak, but the fact that ptr->s->options seems to not be a valid pointer at that point. > At least, this is what valgrind suggested, if I am not wrong or interpreting wrongly the output. > realloc(ptr->s) won't invalidate ptr->s->options. That must happened somewhere else before. By the way, commit with relevant valgrind logs recorded can be helpful. Regards, yousong
Hello!
First of all, thank you very very much for your patience and review.
You are right, my patch introduces an unintended change in behaviour, and actually does not solve the problem.
That day I didn't see this.
However, I think it is still useful to report what happens here, even just for the sole purpose of understanding it, and learning something for me. :)
[mrkiko@gatosaldo build]$ # both system installed uci ("uci"), and the modified one, have been built now. Some infos:
[mrkiko@gatosaldo build]$ cat /etc/config/normalcfg
config normal_section ns
option a 1
config normal_section ns2
option b 2
[mrkiko@gatosaldo build]$ cat /etc/config/exmcfg
config example_section "secttest"
option a 1
option b 2
option c 4
config example_section "secttest"
option d 4
option e 5
[mrkiko@gatosaldo build]$ cat /tmp/r/valgrind.sh
#!/bin/sh
valgrind --suppressions=/tmp/valgrind.suppressions --leak-check=full --leak-resolution=high --show-reachable=no --log-file=valgrind.out $@
[mrkiko@gatosaldo build]$ # system installed uci
[mrkiko@gatosaldo build]$ uci show normalcfg
normalcfg.ns=normal_section
normalcfg.ns.a='1'
normalcfg.ns2=normal_section
normalcfg.ns2.b='2'
[mrkiko@gatosaldo build]$ /tmp/r/valgrind.sh uci show normalcfg
normalcfg.ns=normal_section
normalcfg.ns.a='1'
normalcfg.ns2=normal_section
normalcfg.ns2.b='2'
[mrkiko@gatosaldo build]$ cat valgrind.out
==12860== Memcheck, a memory error detector
==12860== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==12860== Using Valgrind-3.14.0.GIT and LibVEX; rerun with -h for copyright info
==12860== Command: uci show normalcfg
==12860== Parent PID: 12859
==12860==
==12860==
==12860== HEAP SUMMARY:
==12860== in use at exit: 0 bytes in 0 blocks
==12860== total heap usage: 24 allocs, 24 frees, 6,315 bytes allocated
==12860==
==12860== All heap blocks were freed -- no leaks are possible
==12860==
==12860== For counts of detected and suppressed errors, rerun with: -v
==12860== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
[mrkiko@gatosaldo build]$ uci show exmcfg
exmcfg.secttest=example_section
exmcfg.secttest.a='1'
exmcfg.secttest.b='2'
exmcfg.secttest.c='4'
exmcfg.secttest.d='4'
exmcfg.secttest.e='5'
[mrkiko@gatosaldo build]$ /tmp/r/valgrind.sh uci show exmcfg
/tmp/r/valgrind.sh: line 2: 12864 Segmentation fault (core dumped) valgrind --suppressions=/tmp/valgrind.suppressions --leak-check=full --leak-resolution=high --show-reachable=no --log-file=valgrind.out $@
[mrkiko@gatosaldo build]$ cat valgrind.out
==12864== Memcheck, a memory error detector
==12864== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==12864== Using Valgrind-3.14.0.GIT and LibVEX; rerun with -h for copyright info
==12864== Command: uci show exmcfg
==12864== Parent PID: 12863
==12864==
==12864== Invalid read of size 4
==12864== at 0x486E601: uci_lookup_list (list.c:280)
==12864== by 0x4870A0D: uci_parse_option (file.c:478)
==12864== by 0x4871247: uci_parse_line (file.c:530)
==12864== by 0x4871247: uci_import (file.c:680)
==12864== by 0x487195B: uci_file_load (file.c:910)
==12864== by 0x486F10C: uci_load (libuci.c:216)
==12864== by 0x486F240: uci_lookup_ptr (list.c:394)
==12864== by 0x109787: package_cmd (cli.c:312)
==12864== by 0x10A1EF: uci_do_package_cmd (cli.c:422)
==12864== by 0x10A1EF: uci_cmd (cli.c:674)
==12864== by 0x1092CF: main (cli.c:767)
==12864== Address 0x4a6759c is 28 bytes inside a block of size 52 free'd
==12864== at 0x4837471: realloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==12864== by 0x4871A7C: uci_realloc (util.c:49)
==12864== by 0x486FDA5: uci_set (list.c:717)
==12864== by 0x487118D: uci_parse_config (file.c:448)
==12864== by 0x487118D: uci_parse_line (file.c:518)
==12864== by 0x487118D: uci_import (file.c:680)
==12864== by 0x487195B: uci_file_load (file.c:910)
==12864== by 0x486F10C: uci_load (libuci.c:216)
==12864== by 0x486F240: uci_lookup_ptr (list.c:394)
==12864== by 0x109787: package_cmd (cli.c:312)
==12864== by 0x10A1EF: uci_do_package_cmd (cli.c:422)
==12864== by 0x10A1EF: uci_cmd (cli.c:674)
==12864== by 0x1092CF: main (cli.c:767)
==12864== Block was alloc'd at
==12864== at 0x4835558: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==12864== by 0x4871A31: uci_malloc (util.c:39)
==12864== by 0x486E1B2: uci_alloc_generic (list.c:50)
==12864== by 0x486E2EE: uci_alloc_section (list.c:194)
==12864== by 0x486FCF2: uci_set (list.c:699)
==12864== by 0x487118D: uci_parse_config (file.c:448)
==12864== by 0x487118D: uci_parse_line (file.c:518)
==12864== by 0x487118D: uci_import (file.c:680)
==12864== by 0x487195B: uci_file_load (file.c:910)
==12864== by 0x486F10C: uci_load (libuci.c:216)
==12864== by 0x486F240: uci_lookup_ptr (list.c:394)
==12864== by 0x109787: package_cmd (cli.c:312)
==12864== by 0x10A1EF: uci_do_package_cmd (cli.c:422)
==12864== by 0x10A1EF: uci_cmd (cli.c:674)
==12864== by 0x1092CF: main (cli.c:767)
==12864==
==12864== Invalid read of size 1
==12864== at 0x4838A15: strcmp (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==12864== by 0x486E609: uci_lookup_list (list.c:280)
==12864== by 0x4870A0D: uci_parse_option (file.c:478)
==12864== by 0x4871247: uci_parse_line (file.c:530)
==12864== by 0x4871247: uci_import (file.c:680)
==12864== by 0x487195B: uci_file_load (file.c:910)
==12864== by 0x486F10C: uci_load (libuci.c:216)
==12864== by 0x486F240: uci_lookup_ptr (list.c:394)
==12864== by 0x109787: package_cmd (cli.c:312)
==12864== by 0x10A1EF: uci_do_package_cmd (cli.c:422)
==12864== by 0x10A1EF: uci_cmd (cli.c:674)
==12864== by 0x1092CF: main (cli.c:767)
==12864== Address 0x0 is not stack'd, malloc'd or (recently) free'd
==12864==
==12864==
==12864== Process terminating with default action of signal 11 (SIGSEGV): dumping core
==12864== Access not within mapped region at address 0x0
==12864== at 0x4838A15: strcmp (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==12864== by 0x486E609: uci_lookup_list (list.c:280)
==12864== by 0x4870A0D: uci_parse_option (file.c:478)
==12864== by 0x4871247: uci_parse_line (file.c:530)
==12864== by 0x4871247: uci_import (file.c:680)
==12864== by 0x487195B: uci_file_load (file.c:910)
==12864== by 0x486F10C: uci_load (libuci.c:216)
==12864== by 0x486F240: uci_lookup_ptr (list.c:394)
==12864== by 0x109787: package_cmd (cli.c:312)
==12864== by 0x10A1EF: uci_do_package_cmd (cli.c:422)
==12864== by 0x10A1EF: uci_cmd (cli.c:674)
==12864== by 0x1092CF: main (cli.c:767)
==12864== If you believe this happened as a result of a stack
==12864== overflow in your program's main thread (unlikely but
==12864== possible), you can try to increase the size of the
==12864== main thread stack using the --main-stacksize= flag.
==12864== The main thread stack size used in this run was 8388608.
==12864==
==12864== HEAP SUMMARY:
==12864== in use at exit: 957 bytes in 18 blocks
==12864== total heap usage: 21 allocs, 3 frees, 5,137 bytes allocated
==12864==
==12864== LEAK SUMMARY:
==12864== definitely lost: 0 bytes in 0 blocks
==12864== indirectly lost: 0 bytes in 0 blocks
==12864== possibly lost: 0 bytes in 0 blocks
==12864== still reachable: 957 bytes in 18 blocks
==12864== suppressed: 0 bytes in 0 blocks
==12864== Reachable blocks (those to which a pointer was found) are not shown.
==12864== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==12864==
==12864== For counts of detected and suppressed errors, rerun with: -v
==12864== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)
[mrkiko@gatosaldo build]$ # patched uci
[mrkiko@gatosaldo build]$ ./uci show normalcfg
normalcfg.ns=normal_section
normalcfg.ns.a='1'
normalcfg.ns2=normal_section
normalcfg.ns2.b='2'
[mrkiko@gatosaldo build]$ /tmp/r/valgrind.sh ./uci show normalcfg
normalcfg.ns=normal_section
normalcfg.ns.a='1'
normalcfg.ns2=normal_section
normalcfg.ns2.b='2'
[mrkiko@gatosaldo build]$ ./uci show exmcfg # **** unintended behaviour change ****
exmcfg.secttest=example_section
exmcfg.secttest.d='4'
exmcfg.secttest.e='5'
[mrkiko@gatosaldo build]$ /tmp/r/valgrind.sh ./uci show exmcfg
exmcfg.secttest=example_section
exmcfg.secttest.d='4'
exmcfg.secttest.e='5'
[mrkiko@gatosaldo build]$ cat valgrind.out
==12878== Memcheck, a memory error detector
==12878== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==12878== Using Valgrind-3.14.0.GIT and LibVEX; rerun with -h for copyright info
==12878== Command: ./uci show exmcfg
==12878== Parent PID: 12877
==12878==
==12878==
==12878== HEAP SUMMARY:
==12878== in use at exit: 108 bytes in 6 blocks
==12878== total heap usage: 31 allocs, 25 frees, 6,479 bytes allocated
==12878==
==12878== 108 (34 direct, 74 indirect) bytes in 1 blocks are definitely lost in loss record 3 of 3
==12878== at 0x4835558: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==12878== by 0x4846E69: uci_malloc (util.c:39)
==12878== by 0x4843621: uci_alloc_generic (list.c:50)
==12878== by 0x4843845: uci_alloc_option (list.c:85)
==12878== by 0x484512A: uci_set (list.c:696)
==12878== by 0x4845E85: uci_parse_option (file.c:488)
==12878== by 0x484667C: uci_parse_line (file.c:530)
==12878== by 0x484667C: uci_import (file.c:680)
==12878== by 0x4846D95: uci_file_load (file.c:910)
==12878== by 0x4844579: uci_load (libuci.c:216)
==12878== by 0x48446A7: uci_lookup_ptr (list.c:394)
==12878== by 0x109A48: package_cmd (cli.c:312)
==12878== by 0x10A475: uci_do_package_cmd (cli.c:422)
==12878== by 0x10A475: uci_cmd (cli.c:674)
==12878==
==12878== LEAK SUMMARY:
==12878== definitely lost: 34 bytes in 1 blocks
==12878== indirectly lost: 74 bytes in 5 blocks
==12878== possibly lost: 0 bytes in 0 blocks
==12878== still reachable: 0 bytes in 0 blocks
==12878== suppressed: 0 bytes in 0 blocks
==12878==
==12878== For counts of detected and suppressed errors, rerun with: -v
==12878== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)
[mrkiko@gatosaldo build]$ exit
I guess I introduced this leak.
Some problems where still detectable with Valgrind 3.13. but I don't exactly remember.
This is a glibc-based system.
Thanks again for all the help,
Enrico
diff --git a/list.c b/list.c index 25aec56..d934216 100644 --- a/list.c +++ b/list.c @@ -717,6 +717,7 @@ int uci_set(struct uci_context *ctx, struct uci_ptr *ptr) 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); + uci_list_init(&ptr->s->options); } else { free(ptr->s->type); }
If a new section with the same name and type of an old one is found, a memory reallocation happens. Still, the options list for the section is not reinitialized, hence a stale pointer is being used. Signed-off-by: Enrico Mioso <mrkiko.rs@gmail.com> --- list.c | 1 + 1 file changed, 1 insertion(+)