On Fri, Jan 11, 2019 at 08:23:36AM +0100, Martijn van Duren wrote:
> On 1/10/19 6:13 PM, Martijn van Duren wrote:
> > On 1/10/19 1:00 PM, Otto Moerbeek wrote:
> >> On Thu, Jan 10, 2019 at 02:18:50AM -0700, Anthony J. Bentley wrote:
> >>
> >>> Hi,
> >>>
> >>> I have a vi test case that reliably crashes.
> >>>
> >>> It consists of two files. To crash in a 80x24 terminal, the files should
> >>> be as follows:
> >>>
> >>> foo.txt should be 27 lines long. The fifth line should consist of 81
> >>> characters, and the others 80 or below.
> >>>
> >>> bar.txt should be 5 lines or more, each line of less than 80 characters.
> >>>
> >>> Example foo.txt:
> >>> a
> >>> b
> >>> c
> >>> d
> >>> 123456789012345678901234567890123456789012345678901234567890123456789012345678901
> >>> f
> >>> g
> >>> h
> >>> i
> >>> j
> >>> k
> >>> l
> >>> m
> >>> n
> >>> o
> >>> p
> >>> q
> >>> r
> >>> s
> >>> t
> >>> u
> >>> v
> >>> w
> >>> x
> >>> y
> >>> z
> >>> A
> >>>
> >>> Example bar.txt:
> >>> 1
> >>> 2
> >>> 3
> >>> 4
> >>> 5
> >>> 6
> >>>
> >>> Start with "vi foo.txt bar.txt".
> >>> In foo, hit ^F once, then :n to switch files.
> >>> In bar, line 6 is not visible (bug). Hitting ^F here will crash vi.
> >>>
> >>> This might be related to the fact that after the first ^F, foo's long
> >>> line is split, with only the second half shown on screen. The crash
> >>> occurs when trying to return the logical column of the cursor:
> >>>
> >>> #0 0x00000733ac5cbda6 in vs_column (sp=0x73623e317b0, colp=0x73623e31850)
> >>> at /usr/src/usr.bin/vi/build/../vi/vs_relative.c:40
> >>> #1 0x00000733ac5c5c15 in vi (spp=0x7f7ffffbfdc0)
> >>> at /usr/src/usr.bin/vi/build/../vi/vi.c:110
> >>> #2 0x00000733ac5ae0b4 in editor (gp=0x7363966e5f0, argc=<optimized out>,
> >>> argv=<optimized out>) at
> >>> /usr/src/usr.bin/vi/build/../common/main.c:427
> >>> #3 0x00000733ac58e6ce in main (argc=3, argv=0x7f7ffffc0008)
> >>> at /usr/src/usr.bin/vi/build/../cl/cl_main.c:93
> >>>
> >>> --
> >>> Anthony J. Bentley
> >>>
> >>
> >> If you enable DEBUG, the abort in vs_refresh.c:695 gets triggered.
> >> Note that not only line 6 is not drawn, also the ~ lines are missing.
> >> Not hitting ^F but walking to the end or going to the last line with G
> >> in bar works...
> >>
> >> No further clue yet.
> >>
> >> -Otto
> >>
> > I don't have a solution just yet, but I found the culprit.
> >
> > When doing the ^F it places the top of the top of the screen at the
> > overflow of the long line. This position causes HMAP->soff to be set to
> > 2, which isn't reset when loading the new file and causes the soff on
> > all the lines to increment by 1 compared to the previous line, so line
> > 2 is placed at 2 + 3 = 5, line 3 at 3 + 4 = 7, ...
> > This isn't a full explanation yet, since I haven't figured out yet why
> > line 1-5 are printed correctly and I also don't know why scrolling down
> > causes the file to be reset correctly or even exactly what happens when
> > we ^F on the incorrectly printed file, but it's a start.
> >
> > The following (not to be committed) diff proves that the soff is indeed
> > the culprit, but the math is still to obscure for me to say where the
> > fix really should be, which could be somewhere entirely different.
> >
> > martijn@
> >
> So this is how far I'm willing to go for today:
> When entering vi/vs_refresh.c:326 HMAP->lno says that there's 5 lines,
> which get returned. I haven't been able to trace where this value is
> set so far, but it's obviously related to how the HMAP->soff is set to
> 2.
>
> Considering the different ways vi may switch files I found that that
> vi only test for this on vi/vi.c:396. So the diff below resets the
> HMAP state there. Only HMAP should be reset, because the other SMAP
> fields are derived from that one (see vs_sm_{next,prev}. This is
> also backed by vi/vi.c:965, which only initializes HMAP, and not all
> the other maps.
>
> I also tested the diff with a small number of args and opening
> files in split screen and I didn't notice any issues.
>
> I'm somewhat confident that this is the correct solution, but I'm
> not 100% certain, considering Rapunzel probably has an easier time
> untangling her hair.
>
> thoughts?
This works indeed in my tests as well.
-Otto
>
> martijn@
>
> Index: vi/vi.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/vi/vi/vi.c,v
> retrieving revision 1.21
> diff -u -p -r1.21 vi.c
> --- vi/vi.c 18 Apr 2017 01:45:35 -0000 1.21
> +++ vi/vi.c 11 Jan 2019 07:18:39 -0000
> @@ -396,6 +396,10 @@ intr: CLR_INTERRUPT(sp);
> if (F_ISSET(sp, SC_FSWITCH)) {
> F_CLR(sp, SC_FSWITCH);
> (void)sp->gp->scr_rename(sp, sp->frp->name, 1);
> + /* If we switch files, we must draw a fresh screen. */
> + HMAP->lno = sp->lno;
> + HMAP->coff = 0;
> + HMAP->soff = 1;
> }
>
> /* If leaving vi, return to the main editor loop. */
>
