Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Nov 3, 2022

Conversation

leres
Copy link
Collaborator

@leres leres commented Nov 2, 2022

... 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.

... due to size_t being unsigned and a loop that checks for > 0
which can never happen.
@leres
Copy link
Collaborator Author

leres commented Nov 2, 2022

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
Copy link
Collaborator Author

leres commented Nov 2, 2022

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
Copy link
Owner

lichray commented Nov 3, 2022

I prefer

if (--len < 1)
    break;

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

@lichray lichray merged commit 82567f7 into lichray:main Nov 3, 2022
@lichray
Copy link
Owner

lichray commented Nov 3, 2022

Thanks for the fix!

freebsd-git pushed a commit to freebsd/freebsd-ports that referenced this pull request Nov 3, 2022
If you create a tags file of a macro that ends with a '\' and tag
for it, vi dumps core. For example:

    zinc 76 % cat test.h
    #define LATIN2PLAIN(ch) (((u_char)ch) >= 0x80 ? \
       pgm_read_byte_far(pgm_get_far_address(latin2plain) + \
       (((u_char)ch) - 0x80)) : (isprint(ch) ? (ch) : '_'))
    zinc 77 % ctags test.h
    zinc 78 % vi -t LATIN2PLAIN
    Segmentation fault

The problem is that the loop variable is unsigned (size_t) and it
gets decremented twice: 1 -> 0 -> 4294967295

Apply the upstream patch to solve this:

    lichray/nvi2#111
johnsonjh added a commit to johnsonjh/OpenVi that referenced this pull request Feb 26, 2023
* Fix core dump when tags file pattern has a
  trailing `\` due to size_t being unsigned and
  a loop that checks for > 0 which can never happen.

* Revert back to `size_t` and add a check to prevent
  us from decrementing below zero.

* From `nvi2` via lichray/nvi2#111.

Co-authored-by: Craig Leres <leres@FreeBSD.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants