[BUG] dash -c 'echo test > "${1%.in}"' sh /tmp/META.in segfaults on master

After a Git bisection and thorough debugging I found the bad commit to be
c5bf9702ea110bede687b57c0b5fa3fd0e15829e (expand: Add multi-byte support to
pmatch).

The command line that causes the SEGFAULT is
     
     dash -c 'echo test > "${1%.in}"' sh /tmp/META.in

I tested dash from the aforementioned commit with this command line.

The bug is "fixed" if the macro HAVE_FNMATCH is defined. The macro is controlled
by a default-disabled configure '--enable-fnmatch' switch.

The bug is in the src/expand.c::scanright() function (scanleft() seems to be
alright). Essentially, the isssue it that the function returns a pointer to
string A, but later in subevalvar() that pointer is subtracted from the base of
string B. This produces a negative integer which is later happily passed as a
parameter to memmove:

     // startp and rmesc are both char *. scan is a pointer to scanright or
     // scanleft depending on which side the text is trimmed from. In this bug,
     // scan points to scanright().
     loc = scan(startp, endp, rmesc, rmescend, str, quotes, zero);

     // quotes == 0, zero != 0
	if (!loc) {
		if (quotes) {
			rmesc = startp;
			rmescend = endp;
		}
	} else if (!quotes) {
		if (zero)
			rmesc = loc;
		else
             // Control flow goes here: rmescend now points to a substring 
             // of startp.
			rmescend = loc;
	} else if (zero) {
		rmesc = loc;
		rmescend = endp;
	} else {
		rmesc = startp;
		rmescend = loc;
	}

     // Due to the way things are allocated in dahs, startp < rmesc, thus
     // loc - rmesc evaluates to a negative integer.
     //
     // This overwrites a big chunk of memory including, the first argument 
     // of expredir(). This is where the SEGFAULT has been reported.
     // The overwrite causes the function dereference an invalid memory address
     // on the next iteration of for loop.
	memmove(startp, rmesc, rmescend - rmesc /* this is < 0 */);

(Comments have been added by me).

scanright() function look for a needle in a haystack, either from the start of
from the end of the haystack. The function receives two pairs of char pointers
(start + end), i.e. two haystacks of almost identical content, {start,end}p and
rmesc{,end}. Apart from two other non-essential parameters, the function also
receives str, the needle.

The first pair of haystack parameters specifies a pattern which can contain some
special escapes and rmesc points to string with these escapes removed:

     startp: "\201/tmp\201/META.in"
     rmesc:  "/tmp/META.in"

If FNMATCH_IS_ENABLED does not evaluate to zero (FNMATCH_IS_ENABLED depends on
HAVE_FNMATCH, i.e. FNMATCH_IS_ENABLED = 0 by default), the function iterates
over startp and return the substring of startp, not rmesc. But the calling
subevalvar() assumes the the returned value is a substring of rmesc, not startp:

     startp: 0x55555557c9b8 "\201/tmp\201/META.in"
                                              ^
             0x55555557c9c3   pointer returned from scanright()

     rmesc:  0x55555557c9cb "/tmp/META.in"
                                      ^
             0x55555557c9d4    expected pointer

subevalvar() calculates the number of bytes to transfer by (in this case)
subtracting the returned pointer loc from rmesc: 

     0x55555557c9c3 - 0x55555557c9cb

This evaluates to -8 (you can check that yourself) and, when passed as a third
argument to memmove below, we have our good ol' buffer overwrite:

=================================================================
==8694==ERROR: AddressSanitizer: negative-size-param: (size=-8)
     #0 0x7f316ed3b9ed in memmove (/usr/lib/gcc/x86_64-pc-linux-gnu/14/libasan.so.8+0x13b9ed)
     #1 0x5617237b53f4 in subevalvar (/home/master/dev/dash/build/src/dash+0x213f4)
     #2 0x5617237b57a1 in evalvar (/home/master/dev/dash/build/src/dash+0x217a1)
     #3 0x5617237b3c42 in argstr (/home/master/dev/dash/build/src/dash+0x1fc42)
     #4 0x5617237b35e0 in expandarg (/home/master/dev/dash/build/src/dash+0x1f5e0)
     #5 0x5617237abb92 in expredir (/home/master/dev/dash/build/src/dash+0x17b92)
     #6 0x5617237ad944 in evalcommand (/home/master/dev/dash/build/src/dash+0x19944)
     #7 0x5617237aa910 in evaltree (/home/master/dev/dash/build/src/dash+0x16910)
     #8 0x5617237aa236 in evalstring (/home/master/dev/dash/build/src/dash+0x16236)
     #9 0x5617237c4d86 in main (/home/master/dev/dash/build/src/dash+0x30d86)
     #10 0x7f316ea345cd  (/usr/lib64/libc.so.6+0x265cd)
     #11 0x7f316ea34688 in __libc_start_main (/usr/lib64/libc.so.6+0x26688)
     #12 0x5617237a5e94 in _start (/home/master/dev/dash/build/src/dash+0x11e94)

0x56172380022b is located 331 bytes inside of global variable 'stackbase' defined in '../../src/memalloc.c:110:20' (0x5617238000e0) of size 512
SUMMARY: AddressSanitizer: negative-size-param (/usr/lib/gcc/x86_64-pc-linux-gnu/14/libasan.so.8+0x13b9ed) in memmove
==8694==ABORTING


Currently, I am working on a patch. Should come up with something soon-ish.

-- 
Zurab Kvachadze