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);
> >>>           /*
> >>>
> 

Reply via email to