Re: mg: crash on re-search with a file containing an empty line

[Hiltjo Posthuma]

On Sun, Jun 28, 2020 at 08:10:05PM +0200, Theo Buehler wrote:
> On Sun, Jun 28, 2020 at 06:58:22PM +0200, Hiltjo Posthuma wrote:
> > Hi,
> > 
> > I noticed a bug in mg with regex search.
> > 
> > A way to reproduce the issue:
> > 
> > * Create a file with atleast one empty line, for example:
> > 
> > "a
> > 
> > b"
> > 
> > * Move the cursor after "a".
> > * M-x re-search-forward, use a term like "^$".
> > 
> > Result: Segmentation fault (core dumped)
> > 
> > Reproduced on -current on amd64.
> 
> That's because regexec() doesn't like to be passed NULL for its string
> argument. This happens for an empty line as ltext(clp) is NULL in that
> case.
> 
> Diff below is a quick workaround for this problem and marks the places
> where similar crashes can be produced. Perhaps it would be better to
> check for llength(clp) instead of ltext(clp)?
> 
> Index: re_search.c
> ===================================================================
> RCS file: /var/cvs/src/usr.bin/mg/re_search.c,v
> retrieving revision 1.33
> diff -u -p -r1.33 re_search.c
> --- re_search.c       6 Aug 2017 04:39:45 -0000       1.33
> +++ re_search.c       28 Jun 2020 18:05:00 -0000
> @@ -332,8 +332,8 @@ re_forwsrch(void)
>       while (clp != (curbp->b_headp)) {
>               regex_match[0].rm_so = tbo;
>               regex_match[0].rm_eo = llength(clp);
> -             error = regexec(&regex_buff, ltext(clp), RE_NMATCH, regex_match,
> -                 REG_STARTEND);
> +             error = regexec(&regex_buff, ltext(clp) ? ltext(clp) : "",
> +                 RE_NMATCH, regex_match, REG_STARTEND);
>               if (error != 0) {
>                       clp = lforw(clp);
>                       tdotline++;
> @@ -389,8 +389,9 @@ re_backsrch(void)
>                * do this character-by-character after the first match since
>                * POSIX regexps don't give you a way to do reverse matches.
>                */
> -             while (!regexec(&regex_buff, ltext(clp), RE_NMATCH, regex_match,
> -                 REG_STARTEND) && regex_match[0].rm_so < tbo) {
> +             while (!regexec(&regex_buff, ltext(clp) ? ltext(clp) : "",
> +                 RE_NMATCH, regex_match, REG_STARTEND) &&
> +                 regex_match[0].rm_so < tbo) {
>                       memcpy(&lastmatch, &regex_match[0], sizeof(regmatch_t));
>                       regex_match[0].rm_so++;
>                       regex_match[0].rm_eo = llength(clp);
> @@ -538,8 +539,8 @@ killmatches(int cond)
>               /* see if line matches */
>               regex_match[0].rm_so = 0;
>               regex_match[0].rm_eo = llength(clp);
> -             error = regexec(&regex_buff, ltext(clp), RE_NMATCH, regex_match,
> -                 REG_STARTEND);
> +             error = regexec(&regex_buff, ltext(clp) ? ltext(clp) : "",
> +                 RE_NMATCH, regex_match, REG_STARTEND);
>  
>               /* Delete line when appropriate */
>               if ((cond == FALSE && error) || (cond == TRUE && !error)) {
> @@ -613,8 +614,8 @@ countmatches(int cond)
>               /* see if line matches */
>               regex_match[0].rm_so = 0;
>               regex_match[0].rm_eo = llength(clp);
> -             error = regexec(&regex_buff, ltext(clp), RE_NMATCH, regex_match,
> -                 REG_STARTEND);
> +             error = regexec(&regex_buff, ltext(clp) ? ltext(clp) : "",
> +                 RE_NMATCH, regex_match, REG_STARTEND);
>  
>               /* Count line when appropriate */
>               if ((cond == FALSE && error) || (cond == TRUE && !error))
> 

Hi,

Thanks for looking into it, it looks better.

With this patch I noticed re_backsrch() does not work fully correct yet.

I think the line:

> +                 regex_match[0].rm_so < tbo) {

should be:

> +                 regex_match[0].rm_so <= tbo) {

because tbo is already adjusted at the top of the function re_backsrch().

With this line change searching backwards with "^$" finds a result.

-- 
Kind regards,
Hiltjo