On Wed, Aug 02, 2017 at 08:46:26PM +0200, Martijn van Duren wrote: > You're right. Maybe I should read up on my less. :-) > > OK martijn@ for the full diff.
Thanks, committed. > On 08/02/17 19:55, Anton Lindqvist wrote: > > On Wed, Aug 02, 2017 at 07:19:50PM +0200, Martijn van Duren wrote: > >> OK martijn@ for the pattern.c part. > >> > >> The search.c part seems unneeded. > >> - If we don't have a match we don't enter hilite_line at all. > >> - If we have a match and don't negate get sp/ep for every match. > >> - If we have a match and negate, with your process.c patch we get a > >> sp/ep with value NULL and return before entering the do/while. > >> > >> So with the above there's no situation where we can get in the > >> do/while loop and still need to check for NULL on every pass, and the > >> move is thus unnecessary churn. > > > > The changes to pattern.c do solve the initial problem as reported by > > Larry. However, I'm experiencing a crash if I remove the changes to > > search.c: > > > > $ cat in > > foo > > bar > > bar > > $ less in # type: /!foo<CR> > > > >> > >> On 07/28/17 13:32, Anton Lindqvist wrote: > >>> On Fri, Jul 28, 2017 at 01:19:39PM +0200, Theo Buehler wrote: > >>>> On Thu, Jul 27, 2017 at 10:31:51AM +0100, Larry Hynes wrote: > >>>>> Hi > >>>>> > >>>>> $ env | grep LESS > >>>>> LESSHISTFILE=- > >>>>> LESS="-i -M -R -g -c" > >>>>> LESSCHARSET=utf-8 > >>>>> > >>>>> $ unset LESS > >>>>> $ unset LESSCHARSET > >>>>> $ unset LESSHISTFILE > >>>>> > >>>>> $ LESS="-g" > >>>>> $ echo 'foo\nbar\nfoo\nbar\nfoo\nbar' | less > >>>>> > >>>>> While in less, '/' to search, then '^N', or '!', to search for lines > >>>>> which do NOT match the pattern, entering foo<CR> results in a seg > >>>>> fault. > >>>>> > >>>>> This is on amd64. > >>>> > >>>> Thanks for the report. Of course you want 'export LESS="-g"' here, > >>>> otherwise less(1) won't use the -g flag which is crucial for this crash. > >>>> > >>>> I don't have a fix, but maybe this helps a bit: > >>>> > >>>> From the command line, without environment variable magic and no > >>>> interactive component you can reproduce as follows: > >>>> > >>>> $ echo 'foo\nbar\nfoo\n' > crash > >>>> $ less -g -p '!foo' crash > >>>> > >>>> It's apparently a use-after-free that happens in search_range(): > >>>> > >>>> Program terminated with signal SIGSEGV, Segmentation fault. > >>>> #0 0x000009d426e1b35b in create_hilites (linepos=4, > >>>> start_index=-370944992, end_index=-370944989, chpos=0x9d6c5994c80) > >>>> at /usr/src/usr.bin/less/less/../search.c:445 > >>>> 445 hl->hl_startpos = linepos + chpos[start_index]; > >>>> (gdb) bt > >>>> #0 0x000009d426e1b35b in create_hilites (linepos=4, > >>>> start_index=-370944992, end_index=-370944989, chpos=0x9d6c5994c80) > >>>> at /usr/src/usr.bin/less/less/../search.c:445 > >>>> #1 0x000009d426e1b24c in hilite_line (linepos=4, line=0x9d65872bf70 > >>>> "bar", line_len=3, chpos=0x9d6c5994c80, > >>>> sp=0x9d642569390 '\337' <repeats 15 times>, <incomplete sequence > >>>> \337>, > >>>> ep=0x9d642569393 '\337' <repeats 12 times>, <incomplete sequence > >>>> \337>) at /usr/src/usr.bin/less/less/../search.c:494 > >>>> #2 0x000009d426e1aa99 in search_range (pos=8, endpos=-1, > >>>> search_type=257, matches=0, maxlines=-1, plinepos=0x7f7ffffe9ff0, > >>>> pendpos=0x0) at /usr/src/usr.bin/less/less/../search.c:804 > >>>> #3 0x000009d426e1a258 in search (search_type=257, pattern=0x9d42702ca40 > >>>> <cmdbuf> "foo", n=1) > >>>> at /usr/src/usr.bin/less/less/../search.c:925 > >>>> #4 0x000009d426e0a182 in multi_search (pattern=0x9d42702ca40 <cmdbuf> > >>>> "foo", n=1) at /usr/src/usr.bin/less/less/../command.c:815 > >>>> #5 0x000009d426e0a779 in exec_mca () at > >>>> /usr/src/usr.bin/less/less/../command.c:189 > >>>> #6 0x000009d426e09d03 in mca_char (c=10) at > >>>> /usr/src/usr.bin/less/less/../command.c:532 > >>>> #7 0x000009d426e08865 in commands () at > >>>> /usr/src/usr.bin/less/less/../command.c:971 > >>>> #8 0x000009d426e010f2 in main (argc=-1, argv=0x7f7ffffea200) at > >>>> /usr/src/usr.bin/less/less/../main.c:274 > >>>> > >>> > >>> I took a closer look earlier today. As I see it, {start,end}_index does > >>> not get updated inside match_pattern() since we're looking for inverted > >>> matches, i.e. when regexec() indicates no match we actually got a match. > >>> Instead, the indices could be set to NULL up front and the corresponding > >>> NULL-check moved into the loop since match_pattern() is called > >>> repeatedly. Tested with and without `-g` for both normal and inverted > >>> search. > >>> > >>> Index: pattern.c > >>> =================================================================== > >>> RCS file: /cvs/src/usr.bin/less/pattern.c,v > >>> retrieving revision 1.9 > >>> diff -u -p -r1.9 pattern.c > >>> --- pattern.c 21 Nov 2015 13:29:12 -0000 1.9 > >>> +++ pattern.c 28 Jul 2017 11:31:34 -0000 > >>> @@ -122,6 +122,8 @@ match_pattern(void *pattern, char *tpatt > >>> rm.rm_so = 0; > >>> rm.rm_eo = line_len; > >>> #endif > >>> + *sp = NULL; > >>> + *ep = NULL; > >>> matched = !regexec(spattern, line, 1, &rm, flags); > >>> if (matched) { > >>> *sp = line + rm.rm_so; > >>> Index: search.c > >>> =================================================================== > >>> RCS file: /cvs/src/usr.bin/less/search.c,v > >>> retrieving revision 1.18 > >>> diff -u -p -r1.18 search.c > >>> --- search.c 17 Sep 2016 15:06:41 -0000 1.18 > >>> +++ search.c 28 Jul 2017 11:31:34 -0000 > >>> @@ -477,8 +477,6 @@ hilite_line(off_t linepos, char *line, i > >>> char *searchp; > >>> char *line_end = line + line_len; > >>> > >>> - if (sp == NULL || ep == NULL) > >>> - return; > >>> /* > >>> * sp and ep delimit the first match in the line. > >>> * Mark the corresponding file positions, then > >>> @@ -491,6 +489,9 @@ hilite_line(off_t linepos, char *line, i > >>> */ > >>> searchp = line; > >>> do { > >>> + if (sp == NULL || ep == NULL) > >>> + return; > >>> + > >>> create_hilites(linepos, (intptr_t)sp - (intptr_t)line, > >>> (intptr_t)ep - (intptr_t)line, chpos); > >>> /* > >>> >