`-S` can cause heap corruption when viewing some binary files

[ScoreUnder]

What version

$ git rev-parse HEAD
1ca819b

Reproduction steps

1. Open a binary file in less (say yes to the warning)
2. Scroll some way down
3. Type -S to chop long lines

Sometimes this will cause a crash, reported by valgrind as follows:

==2734909== Invalid write of size 1
==2734909==    at 0x11340D: put_wchar (charset.c:700)
==2734909==    by 0x1183B6: cvt_text (cvt.c:95)
==2734909==    by 0x1286AB: pos_shift (position.c:260)
==2734909==    by 0x1286AB: pos_rehead (position.c:300)
==2734909==    by 0x125B1D: toggle_option (option.c:456)
==2734909==    by 0x116E25: mca_opt_char (command.c:519)
==2734909==    by 0x116E25: mca_char (command.c:668)
==2734909==    by 0x116E25: commands (command.c:1360)
==2734909==    by 0x10E99E: main (main.c:475)
==2734909==  Address 0x57a258c is 0 bytes after a block of size 5,244 alloc'd
==2734909==    at 0x484BC13: calloc (vg_replace_malloc.c:1675)
==2734909==    by 0x10ECE8: ecalloc (main.c:512)
==2734909==    by 0x128693: pos_shift (position.c:259)
==2734909==    by 0x128693: pos_rehead (position.c:300)
==2734909==    by 0x125B1D: toggle_option (option.c:456)
==2734909==    by 0x116E25: mca_opt_char (command.c:519)
==2734909==    by 0x116E25: mca_char (command.c:668)
==2734909==    by 0x116E25: commands (command.c:1360)
==2734909==    by 0x10E99E: main (main.c:475)

I can reproduce this with ./less ./less, scrolling down past the first line that says GNU, then typing -S.

[gwsw]

Could you upload or link to a file that exhibits this behavior? I've tried with several binary files, including the compiled version of less, but have been unable to reproduce the crash. Also please note the dimensions of the terminal window you're using when it crashes.

[smemsh]

I reproduced it and saw:

-free(): invalid next size (normal)
Aborted

had to start with -+S to override my $LESS, then scrolled down until it felt "jerky" and did -S --> crash. I don't know what version made that binary though, so I redid it with 1ca819b (master just now) and got:

corrupted size vs. prev_size
Aborted

used ./less -+S less, scrolled down (by single lines) to somewhere just before the "normal" text and used -S. does not happen every time

[smemsh]

oh and

 $ echo $LINES $COLUMNS
44 159

[avih]

It would be best if you can provide the actual binary file which causes the issue for you. Either zip and drag it onto the message, or upload it to someplace which hosts files and post the link here.

Provide the exact steps to reproduce the issue (command line arguments, then which key presses etc).

Preferably the above with $LESS and other LESS env vars unset (run set | grep LESS if you're not sure which vars are set).

[smemsh]

with 1ca819b, if I scroll down exactly 5 lines (same terminal size as above) before using -S it crashes every time, just after the enter needed to remove "chop long lines" message.

[smemsh]

@avih it's just the less from master built with no special options to configure on x86_64. If I unset LESS beforehand, same thing 5 lines, I'm getting:

malloc(): mismatching next->prev_size (unsorted)
Aborted

every time. The file being paged is the less build itself, from within the tree. I can upload it if you can't reproduce given above.

Invocation:

unset LESS # I have no other LESS vars
./less less # from within build tree at 1ca819b3

Keypresses from invocation are:
y ("less may be a binary file. See it anyway?")
j x5 (scroll down 5 lines)
-S (toggle fold)
<enter> (remove "chop long lines" message)
now is when it crashes

[avih]

@avih it's just the less from master built with no special options

Great, so can you upload it please? because it's not identical binary when built on different systems, and it's important to test the exact same file which causes the issue for you, because the developer was not yet able to reproduce the issue.

So please help, as requested more than once, by reducing the number of variable to ensure they can try with YOUR file.

[smemsh]

does not crash on Ubuntu18/aarch64, everything else same

[avih]

Also, it might help to know which operating system you're using.

[smemsh]

I did mention that I would upload it if it can't be reproduced given the additional detail provided. Nonetheless here you go: less.gz

build was
git clone ...
git checkout master
make -f Makefile.aut distfiles
sh configure
make -j4
unset LESS
./less less

Then above key sequence. System is Ubuntu 22 on x86_64 with updates a few weeks old. Terminal 44 lines, 159 columns.

[smemsh]

It's not crashing when built on Ubuntu 20 / x86_64 either. I tried many different scrolling offsets and can't get it to crash there.

However I did a build with LDFLAGS=-static on the u22 system, copied it to the u20 system and there it does crash with same steps. So something specific to the libc or ncurses perhaps.

[gwsw]

Following those instructions it does not crash on a Fedora system. I also built less with the gcc address sanitizer and ran the instructions again and the sanitizer does not report any problem.

However, examining the code, I think I see what the problem might be. Can you apply this patch and let me know if it fixes it for you?

diff --git a/position.c b/position.c
index ee06bac..69ab7a8 100644
--- a/position.c
+++ b/position.c
@@ -256,7 +256,7 @@ static int pos_shift(POSITION linepos, size_t choff)
        return -1;
    cvt_ops = get_cvt_ops(0); /* {{ Passing 0 ignores SRCH_NO_REGEX; does it matter? }} */
    /* {{ It would be nice to be able to call cvt_text with dst=NULL, to avoid need to alloc a useless cline. }} */
-   cline = (char *) ecalloc(1, line_len+1);
+   cline = (char *) ecalloc(1, cvt_length(line_len, cvt_ops));
    cvt_text(cline, line, NULL, &line_len, cvt_ops);
    free(cline);
    return (int) line_len;  /*{{type-issue}}*/

[smemsh]

Here is the static binary built on u22 that crashes, fwiw:
less.static.gz

I will try your patch presently.

[smemsh]

I don't get the crash anymore after applying your patch. I tried many offsets. Good eye!

[ScoreUnder]

With the above patch I get a clean valgrind output even after plenty of exploration of a file switching between fold and chop mode, and no crash any more.