lichray / nvi2

A multibyte fork of the nvi editor for BSD
Other
144 stars 34 forks source link

Fix core dump when tags file pattern has a trailing '\' #111

Closed leres closed 1 year ago

leres commented 1 year ago

... due to size_t being unsigned and a loop that checks for > 0 which is always true (even when the loop variable is decremented twice).

I found that I could crash nvi (both the @lichray fork and the version that comes with the base 13.1-RELEASE system) with a tags file for this macro:

#define LATIN2PLAIN(ch) (((u_char)ch) >= 0x80 ? \
    pgm_read_byte_far(pgm_get_far_address(latin2plain) + \
    (((u_char)ch) - 0x80)) : (isprint(ch) ? (ch) : '_'))

I believe the trigger is the trailing '\'. The crash happens in a loop in re_tag_conv():

/*
 * Escape every other magic character we can find, meanwhile stripping
 * the backslashes ctags inserts when escaping the search delimiter
 * characters.
 */
for (; len > 0; --len) {
        if (p[0] == '\\' && (p[1] == '/' || p[1] == '?')) {
                ++p;
                --len;
        } else if (STRCHR(L("^.[]$*"), p[0]))
                *t++ = '\\';
        *t++ = *p++;
}

The extra --len when len is already zero is the problem.

It's possible size_t was signed on the system nvi was developed. But at least one source claims size_t is "always unsigned".

This PR only addresses the crash I could reproduce by making len an int (large enough for lines in a tags file). But there are more than 100 places where len is declared a size_t and many/most of them have loops with len > 0 as the condition. The simple fix for the rest of these would be to change size_t to int.

Another way to address this would be to look for places where the loop variable is a size_t and the loop variable is decremented inside the loop. But that seems fragile.

leres commented 1 year ago

I did a quick scan and didn't find any actual problems; all other uses appear to be careful when decrementing, for example argv_uescI():

    for (p = bp; len > 0; ++str, --len) {
            if (IS_ESCAPE(sp, excp, *str)) {
                    if (--len < 1)
                            break;
                    ++str;

Let me know if you'd prefer this style fix for this PR.

leres commented 1 year ago

Better example, ex_aci():

for (t = p; len > 0 && t[0] != '\n'; ++t, --len);
        if (t != p || len == 0) {
                if (F_ISSET(sp, SC_EX_GLOBAL) &&
                    t - p == 1 && p[0] == '.') {
                        ++t;
                        if (len > 0)
                                --len;
                        break;
                }
lichray commented 1 year ago

I prefer

if (--len < 1)
    break;

(unless you are in switch). Variables named blen has always been unsigned.

lichray commented 1 year ago

Thanks for the fix!