Obscure "put" bug when spreadsheet needs to grow

[rkrten]

I found a weird bug that I was unable to repro until just now.

How it manifests is that when you delete a row ("dr" command), and then put it somewhere else (via "pr"), sometimes bogus field data shows up in newly put down row.

I tracked it down to being involved with growing the spreadsheet. Let's say the last line is currently line 99. If you go into the spreadsheet and delete a line and the put it somewhere else, the line you just put may become contaminated with data from the last line (esp. if the last line has lots of fields, and the line you've deleted (and are putting) doesn't).

I'm hoping this may tweak something for someone and the fix is easy :-)

[dtkerns]

I've reproduced this ... the corruption actually happens with the "dr" command... but definitely, it is compounded with the "pr". ... I'm VERY new to this project so give me some time to grok the code a bit.

[Arumes]

I can confirm this bug. I hope the text below is clear enough as it's a pretty complex bug, and english is not my native language.

How SC seems to allocate space for rows:

SC seems to initialise the number of rows based on screen/window size and sheet size. If the sheet size equals or is smaller than the number of rows that fit on the screen, the number of rows initially created equals the number of rows required to fill the screen (so usually the number of rows equals the number of screen lines - 3). But there's an absolute minimum initial size of 100 rows, so SC will always initialise with at least 100 rows on small screens and/or small sheets.

If you have a terminal window of 150 lines, it shows 147 rows of the sheet.

• If the sheet size is equal to or more than 147 rows, SC will initialise with the number of rows in the sheet.
• If the sheet size is less than 147 rows, SC will initialise with 147 rows.

If you have a terminal window of 24 lines, it shows 21 rows of the sheet.

• If the sheet size is more than 100 rows, SC will initialise with the number of rows in the sheet.
• If the sheet size is equal to or less than 100 rows, SC will initialise with 100 rows.

Now the bug:

If the last row of the sheet equals the last row of allocated space, you get the dr command bug. Couple of examples:

• If the last row in the sheet exceeds row 98, and the screen size is equal to or less than 103 lines (100 rows), and you haven't scrolled past the last row yet, you get the bug.
• If the terminal has 140 lines, and thus shows 137 rows of the sheet, and the last row of your sheet is 137, and you haven't scrolled past row 137 yet, you get the bug.

If you do scroll past the last allocated row, SC will allocate space for 30 more rows and the bug disappears because the last row of the sheet is not the last row allocated anymore.

So while the 99 in the initial bug report ("let's say the last line is currently line 99") reads like an arbitrary number, this is actually the smallest number of rows where the bug occurs.

Personally I don't think there's a bug in the pr code, that code just handles whatever data the dr code gives it. If that data is corrupt, it's dr's fault. :-) I can't get pr to mess up without dr going wrong first.

[dtkerns]

Yes, I'm quite certain I tried to communicate the same thing in Aug '22. (albeit much less verbosely) I think it's an interaction with the curses library. I've lacked both time and motivation to dig into it, but if you work out a fix, I'll gladly help test and integrate it.

[Arumes]

Yeah I understand, it's not exactly a high priority project. Take it easy, I myself only work on this project in my spare time (and it's not my only hobby) and on coffee brakes during work. The program is over 40 years old, it's not like we have to fix everything in one year now. :-)

And I'm in the same situation as you were then: lacking knowledge of the code base. I already took a look at deleterow() in cmds.c, but for now this is way over my head. I just thought it would be a good idea to explain the bug in more detail so that the person who does get around to fixing it (might be me somewhere in the future), doesn't have to go through that process again.

BTW there's no need for long lines for the dr bug to occur. This bug "works" just as well with just the A column populated, and a single extra cell in the B column on the last row of the sheet. Also, a second pr after the first one copies different garbage, usually consisting of empty rows and hide commands. But that might just be memory corruption caused by the faulty dr code.

As I said, it's a complex bug. I prefer simple buffer overflows like the first one I reported. 😅

[dtkerns]

I was looking at this this past week... I have a one line change that I don't fully understand, but it seems to fix this bug. I'm also unsure of any unintended side effects. It would be nice to have some people try it out and let me know if it is a good fix.
... this is probably not the right place to initialize this, but it's where valgrind complains about it

--- a/cmds.c
+++ b/cmds.c
@@ -1145,6 +1145,7 @@ closerow(int rs, int numrow)
        for (c = 0; c <= maxcol; c++) {
            pp = ATBL(tbl, r, c);
            if (*pp) {
+                (*pp)->nlastrow = 0;