[prev in list] [next in list] [prev in thread] [next in thread]
List: busybox
Subject: Re: sed segfault after commit 9c47c43e0736 with FEATURE_CLEAN_UP
From: Thomas De Schampheleire <patrickdepinguin () gmail ! com>
Date: 2018-09-25 11:02:00
Message-ID: CAAXf6LWuo3oy6vxQFb1+35oFYPcZZ1KJmQX7td2w0Ydkf_ipZQ () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
On Tue, Sep 25, 2018, 12:53 Denys Vlasenko <vda.linux@googlemail.com> wrote:
> On Mon, Sep 24, 2018 at 4:36 PM Thomas De Schampheleire
> <patrickdepinguin@gmail.com> wrote:
> > If FEATURE_CLEAN_UP is enabled, following reproduction test case
> > causes a segfault:
> >
> > +testing "sed uses previous regexp test2" \
> > + "sed -n '/foo/ { //p }'" \
> > + "" \
> > + "" \
> > + "q\nw\ne\nr\n"
> > +
> >
> > (note: the test case passes because the output is correct; it seems
> > the test suite does not catch segfaults)
> >
> > The issue is bisected to commit 9c47c43e0736 which adds support for
> > '//' to indicate the previous regex. The change in question is:
> >
> > - temp = copy_parsing_escapes(pos, next);
> > - *regex = xzalloc(sizeof(regex_t));
> > - xregcomp(*regex, temp, G.regex_type);
> > - free(temp);
> > + if (next != 0) {
> > + temp = copy_parsing_escapes(pos, next);
> > + G.previous_regex_ptr = *regex =
> > xzalloc(sizeof(regex_t));
> > + xregcomp(*regex, temp, G.regex_type);
> > + free(temp);
> > + } else {
> > + *regex = G.previous_regex_ptr;
> > + if (!G.previous_regex_ptr)
> > + bb_error_msg_and_die("no previous
> regexp");
> > + }
> >
> >
> > In the original code, *regex was always a newly allocated pointer.
> > In the new code, there are cases where *regex (mapping to
> > sed_cmd.beg_match in the caller) is not a newly allocated pointer, but
> > the same as the previously allocated one.
> >
> > When sed finishes, and sed_free_and_close_stuff() is called, there is
> > an iteration over the different sed_cmd structures. Following code:
> >
> > if (sed_cmd->beg_match) {
> > regfree(sed_cmd->beg_match);
> > free(sed_cmd->beg_match);
> > }
> >
> > will work fine in the first iteration that touches the repeated
> > pointer, but will fail in 'regfree' on the next iteration, where the
> > beg_match pointer points to the same value as was already freed.
> >
> > Backtrace is:
> > Program received signal SIGSEGV, Segmentation fault.
> > free_token (node=0x198) at regcomp.c:3808
> > 3808 regcomp.c: No such file or directory.
> > (gdb) bt
> > #0 free_token (node=0x198) at regcomp.c:3808
> > #1 0xf6fe474c in free_dfa_content (dfa=0x712220) at regcomp.c:598
> > #2 0xf6ff1200 in __regfree (preg=0x712068) at regcomp.c:647
> > #3 0x000d53dc in sed_free_and_close_stuff () at editors/sed.c:184
> > #4 0xf6f6be94 in __run_exit_handlers (status=0x0, listp=0xf70744b4
> > <__exit_funcs>,
> > run_list_atexit=run_list_atexit@entry=0x1) at exit.c:82
> > #5 0xf6f6bee8 in __GI_exit (status=<optimized out>) at exit.c:104
> > #6 0x0001a194 in xfunc_die () at libbb/xfunc_die.c:20
> > #7 0x000196b0 in run_applet_no_and_exit (applet_no=0xa9,
> > name=0xfff7ce6a "sed", argv=0xfff7bf14)
> > at libbb/appletlib.c:952
> > #8 0x00019718 in run_applet_and_exit (name=0xfff7ce6a "sed",
> > argv=0xfff7bf14) at libbb/appletlib.c:968
> > #9 0x000197fc in main (argc=0x4, argv=0xfff7bf14) at
> libbb/appletlib.c:1076
> >
> >
> > I'm not sure how to fix this:
> > * either keep a list in sed_free_and_close_stuff of pointers already
> freed
> > * or create a separate list of pointers-to-free when allocating
> > pointers rather than assuming that all values in beg_match need to be
> > freed
> > * or add a flag to indicate this special case in sed_cmd, which would
> > need some more communication between get_address and its caller.
> > * something else?
>
> How about "don't free them at all"?
>
Doesn't this contradict with the purpose of FEATURE_CLEAN_UP?
>
[Attachment #5 (text/html)]
<div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr">On Tue, Sep 25, \
2018, 12:53 Denys Vlasenko <<a \
href="mailto:vda.linux@googlemail.com">vda.linux@googlemail.com</a>> \
wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex">On Mon, Sep 24, 2018 at 4:36 PM \
Thomas De Schampheleire<br> <<a href="mailto:patrickdepinguin@gmail.com" \
target="_blank" rel="noreferrer">patrickdepinguin@gmail.com</a>> wrote:<br> > \
If FEATURE_CLEAN_UP is enabled, following reproduction test case<br> > causes a \
segfault:<br> ><br>
> +testing "sed uses previous regexp test2" \<br>
> + "sed -n '/foo/ { //p }'" \<br>
> + "" \<br>
> + "" \<br>
> + "q\nw\ne\nr\n"<br>
> +<br>
><br>
> (note: the test case passes because the output is correct; it seems<br>
> the test suite does not catch segfaults)<br>
><br>
> The issue is bisected to commit 9c47c43e0736 which adds support for<br>
> '//' to indicate the previous regex. The change in question is:<br>
><br>
> - temp = copy_parsing_escapes(pos, next);<br>
> - *regex = xzalloc(sizeof(regex_t));<br>
> - xregcomp(*regex, temp, G.regex_type);<br>
> - free(temp);<br>
> + if (next != 0) {<br>
> + temp = copy_parsing_escapes(pos, next);<br>
> + G.previous_regex_ptr = *regex =<br>
> xzalloc(sizeof(regex_t));<br>
> + xregcomp(*regex, temp, G.regex_type);<br>
> + free(temp);<br>
> + } else {<br>
> + *regex = G.previous_regex_ptr;<br>
> + if (!G.previous_regex_ptr)<br>
> + bb_error_msg_and_die("no \
previous regexp");<br> > + }<br>
><br>
><br>
> In the original code, *regex was always a newly allocated pointer.<br>
> In the new code, there are cases where *regex (mapping to<br>
> sed_cmd.beg_match in the caller) is not a newly allocated pointer, but<br>
> the same as the previously allocated one.<br>
><br>
> When sed finishes, and sed_free_and_close_stuff() is called, there is<br>
> an iteration over the different sed_cmd structures. Following code:<br>
><br>
> if (sed_cmd->beg_match) {<br>
> regfree(sed_cmd->beg_match);<br>
> free(sed_cmd->beg_match);<br>
> }<br>
><br>
> will work fine in the first iteration that touches the repeated<br>
> pointer, but will fail in 'regfree' on the next iteration, where the<br>
> beg_match pointer points to the same value as was already freed.<br>
><br>
> Backtrace is:<br>
> Program received signal SIGSEGV, Segmentation fault.<br>
> free_token (node=0x198) at regcomp.c:3808<br>
> 3808 regcomp.c: No such file or directory.<br>
> (gdb) bt<br>
> #0 free_token (node=0x198) at regcomp.c:3808<br>
> #1 0xf6fe474c in free_dfa_content (dfa=0x712220) at regcomp.c:598<br>
> #2 0xf6ff1200 in __regfree (preg=0x712068) at regcomp.c:647<br>
> #3 0x000d53dc in sed_free_and_close_stuff () at editors/sed.c:184<br>
> #4 0xf6f6be94 in __run_exit_handlers (status=0x0, listp=0xf70744b4<br>
> <__exit_funcs>,<br>
> run_list_atexit=run_list_atexit@entry=0x1) at exit.c:82<br>
> #5 0xf6f6bee8 in __GI_exit (status=<optimized out>) at exit.c:104<br>
> #6 0x0001a194 in xfunc_die () at libbb/xfunc_die.c:20<br>
> #7 0x000196b0 in run_applet_no_and_exit (applet_no=0xa9,<br>
> name=0xfff7ce6a "sed", argv=0xfff7bf14)<br>
> at libbb/appletlib.c:952<br>
> #8 0x00019718 in run_applet_and_exit (name=0xfff7ce6a "sed",<br>
> argv=0xfff7bf14) at libbb/appletlib.c:968<br>
> #9 0x000197fc in main (argc=0x4, argv=0xfff7bf14) at \
libbb/appletlib.c:1076<br> ><br>
><br>
> I'm not sure how to fix this:<br>
> * either keep a list in sed_free_and_close_stuff of pointers already freed<br>
> * or create a separate list of pointers-to-free when allocating<br>
> pointers rather than assuming that all values in beg_match need to be<br>
> freed<br>
> * or add a flag to indicate this special case in sed_cmd, which would<br>
> need some more communication between get_address and its caller.<br>
> * something else?<br>
<br>
How about "don't free them at all"?<br></blockquote></div></div><div \
dir="auto"><br></div><div dir="auto">Doesn't this contradict with the purpose of \
FEATURE_CLEAN_UP?</div><div dir="auto"><div class="gmail_quote"><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"> </blockquote></div></div></div>
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic