bugGNU nano - Bugs: bug #56102, unindenting a partially displayed...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #56102: unindenting a partially displayed line can result in a freeze

Submitter:  easyaspi314 <easyaspi314>
Submitted:  Mon 08 Apr 2019 11:25:30 PM UTC
   
 
Severity:  3 - Normal Status:  Fixed
Assigned to:  bens Open/Closed:  Closed

Jump to the original submission

Tue 16 Apr 2019 08:48:27 AM UTC, comment #19: 

The fix has been released in 4.1.

Benno Schulenberg <bens>
Group administrator
Sat 13 Apr 2019 10:00:35 AM UTC, comment #18: 

Fixed in git, commit 1c707d4f.  (Plus the subsequent commit for the commenting/uncommenting and indenting case.)

Thanks a lot for reporting, and for investigating and coming close to finding the cause.

Benno Schulenberg <bens>
Group administrator
Fri 12 Apr 2019 04:19:45 PM UTC, comment #17: 

Yes, that should work; there are really several problems here that were all interacting.  I'd forgotten about the possibility of firstcolumn's being thrown out of whack by multi-line changes, and it can be difficult to tell when that happens.

And yes, the same problem can occur when commenting/uncommenting.  Justify is multi-line, but it doesn't have the problem since it reuses the cutting and pasting code, which account for this.

David Lawrence Ramsey <dolorous>
Group Member
Fri 12 Apr 2019 12:12:13 PM UTC, comment #16: 

Putting in an 'ensure_firstcolumn_is_aligned()' at the end of do_unindent() fixes the issue for me.  See attached patch.

It could be guarded with an 'if (openfile->current == openfile->edittop || (openfile->mark && openfile->mark->lineno <= openfile->edittop->lineno))' but... will see.

(file #46759)

Benno Schulenberg <bens>
Group administrator
Fri 12 Apr 2019 11:30:21 AM UTC, comment #15: 

With the reproduction recipe from the previous comment, the underflow happens when only the tail part (" error_day)") of line 69 is in the top row of the window.  When then typing M-{ for the second time, the top row becomes blank, which is weird: one would expect the previous chunk to appear.

But it is a little clearer what happens: the softwrapping display routine keeps trying to display the second chunk of line 69, starting from column 69 (72 wide - 3 for the line numbers), and then get_softwrap_breakpoint() comes back with... 64.  But there is no second chunk, firstcolumn is wrong, it should have been zero -- it needs alignment when indenting/unindenting affects a line that is on or before the top row.  The same probably for commenting/uncommenting.

Benno Schulenberg <bens>
Group administrator
Fri 12 Apr 2019 10:16:29 AM UTC, comment #14: 

Yes, can reproduce, in a 31x72 terminal (31 rows, 72 columns).
Run 'src/nano --ignore --soft --line +30 Perishable.cpp', type ^6 and hold down <Down> until exactly on line 91, then one <Up>, then M-{.  Then M-{ again, but this one is ineffective: it hangs.  But it hangs only when running the thing under valgrind.  :|

==18233== Conditional jump or move depends on uninitialised value(s)
==18233==    at 0x52D8A59: mbtowc (mbtowc.c:55)
==18233==    by 0x10F04C: mbwidth (chars.c:241)
==18233==    by 0x12E95B: display_string (winio.c:1998)
==18233==    by 0x13217B: update_softwrapped_line (winio.c:2876)
==18233==    by 0x132290: update_line (winio.c:2784)
==18233==    by 0x1327A6: edit_refresh (winio.c:3327)
==18233==    by 0x121126: main (nano.c:2738)
==18233==  Uninitialised value was created by a heap allocation
==18233==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18233==    by 0x12C918: nmalloc (utils.c:305)
==18233==    by 0x12E577: display_string (winio.c:1889)
==18233==    by 0x13217B: update_softwrapped_line (winio.c:2876)
==18233==    by 0x132290: update_line (winio.c:2784)
==18233==    by 0x1327A6: edit_refresh (winio.c:3327)
==18233==    by 0x121126: main (nano.c:2738)

The hang seems to get prevented by David's safety catch [as seen a few times before: the first run after a program change seems to be using the old binary -- weird], so I will put that in too.  But neither change is enough to stop the above complaint.  The underflow must still be happening.

Benno Schulenberg <bens>
Group administrator
Fri 12 Apr 2019 09:57:05 AM UTC, comment #13: 

At least 'last_chunk' is uninitialized.  But after changing the program to set it to FALSE, I still got:

==13245== Conditional jump or move depends on uninitialised value(s)
==13245==    at 0x131965: actual_last_column (winio.c:3207)
==13245==    by 0x11C2F9: set_proper_index_and_pww (move.c:101)
==13245==    by 0x11CDBB: do_down (move.c:525)
==13245==    by 0x11FD86: do_input (nano.c:1799)
==13245==    by 0x121142: main (nano.c:2747)
==13245==  Uninitialised value was created by a stack allocation
==13245==    at 0x131902: actual_last_column (winio.c:3197)

And experienced a hang.  Which I then cannot reproduce.

Benno Schulenberg <bens>
Group administrator
Fri 12 Apr 2019 09:32:49 AM UTC, comment #12: 

Running the command in valgrind, the first unindent took ages.  After two minutes I gave up.  Result:

==9751== Conditional jump or move depends on uninitialised value(s)
==9751==    at 0x131965: actual_last_column (winio.c:3207)
==9751==    by 0x11C2F9: set_proper_index_and_pww (move.c:101)
==9751==    by 0x11CDBB: do_down (move.c:525)
==9751==    by 0x11FD86: do_input (nano.c:1799)
==9751==    by 0x121142: main (nano.c:2747)
==9751==  Uninitialised value was created by a stack allocation
==9751==    at 0x131902: actual_last_column (winio.c:3197)
==9751==
==9751== Conditional jump or move depends on uninitialised value(s)
==9751==    at 0x52D8A59: mbtowc (mbtowc.c:55)
==9751==    by 0x10F04C: mbwidth (chars.c:241)
==9751==    by 0x12E95B: display_string (winio.c:1998)
==9751==    by 0x132176: update_softwrapped_line (winio.c:2876)
==9751==    by 0x13228B: update_line (winio.c:2784)
==9751==    by 0x1327A1: edit_refresh (winio.c:3327)
==9751==    by 0x121126: main (nano.c:2738)
==9751==  Uninitialised value was created by a heap allocation
==9751==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==9751==    by 0x12C918: nmalloc (utils.c:305)
==9751==    by 0x12E577: display_string (winio.c:1889)
==9751==    by 0x132176: update_softwrapped_line (winio.c:2876)
==9751==    by 0x13228B: update_line (winio.c:2784)
==9751==    by 0x1327A1: edit_refresh (winio.c:3327)
==9751==    by 0x121126: main (nano.c:2738)
==9751==
==9751==
==9751== More than 10000000 total errors detected.  I'm not reporting any more.
==9751== Final error counts will be inaccurate.  Go fix your program!

Benno Schulenberg <bens>
Group administrator
Fri 12 Apr 2019 09:10:37 AM UTC, comment #11: 

With a 74x34 terminal I cannot reproduce the hang either.

But something else did occur.  Running 'nano --ignore --soft --line +30 Perishable.cpp', then typing ^6 and holding down the <Down> arrow key until the cursor is on line 91 and then typing <Up> once, and then unindenting several times (maybe four or five times), and then trying to undo this with M-U, nano crashed: free() invalid pointer, and left the terminal in a locked up state.  But I cannot reproduce this.

Benno Schulenberg <bens>
Group administrator
Wed 10 Apr 2019 01:00:17 PM UTC, comment #10: 

David, your patch does indeed fix the hang.

Benno, try 74x34 which is the dimensions I used. Apparently though it does have something to do with uninitialized memory because it doesn't occur with asan. 🤔

easyaspi314 <easyaspi314>
Wed 10 Apr 2019 10:22:58 AM UTC, comment #9: 

Thanks for the exact series of keystrokes.  However, I am unable to reproduce.  I've tried with 'nano --ignore --linenum --soft Perishable.cpp' on a terminal of 28 rows and 74 columns.  No hang. Probably the hang depends on the precise height of the terminal too?  And maybe other options?

About David's assessment...  First, the loop can never cause 'converted + index' to become negative, it could only cause index to become zero and then stay forever zero -- move_mbleft() will not step to before converted.  Second, the example file does not contain any zero-width characters, so they cannot be causing the problem here.  Third, if a line consists of only zero-width characters, it will not overflow: it requires at least two-columns worth of characters before a line will overflow, and the loop will end as soon as a single non-zero-width character is nibbled off.  Fourth, when softwrap is on, nano should never even be trying to show the ">" continuation character.  The nibbling loop could only be invoked when there is a double-width character straddling two chunks.  But the example file does not contain any double-width character either.

So, the nibbling loop getting invoked (and hanging) when it shouldn't doesn't have anything to do with zero-width or double-width characters, it is caused by 'span' underflowing, making that 'beyond' becomes smaller than 'converted', and that 'converted' never gets any character copied into it, and on Devin's machine the default contents of memory are apparently zero-width characters, causing the hang at index = 0.

We could of course, as a safety measure, to prevent a hang, add an 'index > 0' condition to the 'do' loop, but that would not fix the actual bug: 'span' underflowing.

Benno Schulenberg <bens>
Group administrator
Wed 10 Apr 2019 09:16:06 AM UTC, comment #8: 

It should be the same bug, if the formula you gave to reproduce it is for that bug.  (I've also checked out the video in comment #2.)  Does the underflow still occur with the patch applied?

The root of the problem is that when index is shifted too far backward to the previous line, converted + index is a negative value, which means that when a null character gets inserted at that location, it's in an invalid location, which can cause all kinds of (difficult-to-trace) havoc.

The fact that softwrap has to loop through a given line multiple times to find softwrap breakpoints makes the effects of this more visible, as does the shifting of the line when you unindent.  The bug exists regardless of these, but it's easier to trigger because of them.

David Lawrence Ramsey <dolorous>
Group Member
Wed 10 Apr 2019 03:51:45 AM UTC, comment #7: 

Yeah, as soon as I saw LLDB break there, I was like "yeaaaah that doesn't look right..."

What about the underflow? Because Benno assumes that this case should not happen (despite the numerous underflow checks throughout the function which proves otherwise). Is it the same bug?

easyaspi314 <easyaspi314>
Wed 10 Apr 2019 03:04:13 AM UTC, comment #6: 

The problem is indeed caused by the loop you mention in step 8, which was added in commit 7c30d1f.

It doesn't account for the corner case where the current line consists of zero-width characters, and the previous line ends with a zero-width character.  The loop blindly moves the index backward to the first non-zero-width character, without accounting for the fact that it ends up on the previous line.

The loop should stop at the beginning of the line when trying to move back.  The attached patch against git f1e5da6 should fix it.

(file #46742)

David Lawrence Ramsey <dolorous>
Group Member
Tue 09 Apr 2019 08:30:15 PM UTC, comment #5: 

I have it 100%.

1. Resize your terminal to be small enough to cause wrapping in the lower lines of Perishable.cpp after an unindent. With tab size set to 8, 74 columns works for me.
2. Open Perishable.cpp in nano.
3. Begin your selection at the brace (inclusive) on line 13 under the namespace.
4. Scroll down to the bottom and position your cursor at the blank line under the brace at line 90.


[ ][ ][ ][ ][ ][ ][ ][ ][}]$
[#]$


5. Unindent.
6. Reposition your cursor if needed so that it is at that line. At this point, the cursor should look like this:


[}]$
[#]$


7. Unindent.
8. The cursor goes to the previous line, highlighting the brace, and Nano freezes on this loop:


   1996                 do {
   1997                         index = move_mbleft(converted, index);
-> 1998                 } while (mbwidth(converted + index) == 0);
   1999


For keystrokes, it would be this:
^_
13
Enter
^6
^_
90
Enter
M-{
M-{

easyaspi314 <easyaspi314>
Tue 09 Apr 2019 07:35:28 PM UTC, comment #4: 

How can I tell the expression underflowed? I checked lldb and to_col was 70 and from_col was 77. Therefore, display_string was being called with the value 4294967289.

I do note that it seems to have something to do with moving my cursor.

I'm trying to find a 100% reproduction method,

easyaspi314 <easyaspi314>
Tue 09 Apr 2019 07:09:00 PM UTC, comment #3: 

How did you conclude that 'to_col - from_col' can underflow?

If it is indeed the case, it would mean that the preceding call of get_softwrap_breakpoint() returns a to_col that is smaller than the given from_col, which should not be possible.  It would also mean that it is a display issue, and should be reproducible with a specific file and a specific terminal size, and would not depend upon doing an unindent, no?

Benno Schulenberg <bens>
Group administrator
Tue 09 Apr 2019 12:52:26 AM UTC, comment #2: 

Ok so the video didn't attach and Streamable wasn't cooperating, so I put it on MEGA which unfortunately needs JS which I know Benno is terrified of. Tell me if you need a different upload service. (The website is safe)

https://mega.nz/#!9CpTRKjZ!CA5XofBD6aCKVYB2B_J2r9aEq3mu4seFC046Sqz6o0o


easyaspi314 <easyaspi314>
Tue 09 Apr 2019 12:27:00 AM UTC, comment #1: 

It seems to have something to do with moving the cursor or being next to a character.

I attached the file I produced it on, as well as a screencast of the bug occuring on my LG G3. (Note: when I pressed E on the keyboard, I was pressing VolUp which is a shortcut on Termux to send the escape key; I did it out of habit)

I can get it about 30% of the time this way.

(file #46735)

easyaspi314 <easyaspi314>
Mon 08 Apr 2019 11:25:30 PM UTC, original submission:  


                /* Convert the chunk to its displayable form and draw it. */
                converted = display_string(fileptr->data, from_col, to_col - from_col,
                                                                        TRUE, FALSE);


I ran into a very odd bug which is a little tricky to reproduce (no 100% success reproduction steps yet), but I ensure that this happened multiple times.

I was trying to unindent some text with M-{, and Nano froze.

The faulty line is above (update_softwrapped_line, winio.c:2876 on HEAD)

Apparently, to_col - from_col may occasionally underflow, resulting in an essentially infinite loop as the value jumps to more than 4 billion.

This occurred on HEAD and 4.0. Perhaps a range check would fix this.

easyaspi314 <easyaspi314>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #46735:  Perishable.cpp added by easyaspi314 (2KiB - text/x-c++src - screen cast of issue)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by dolorous (Updated the item)
  • -email is unavailable- added by bens (Posted a comment)
  • -email is unavailable- added by easyaspi314 (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-04-16 bens Open/ClosedOpen Closed
    2019-04-13 bens StatusIn Progress Fixed
        Summaryunindent occasionally causes underflow in update_softwrapped_line() resulting in a freeze unindenting a partially displayed line can result in a freeze
    2019-04-12 bens Attached File- Added 0001-unindent-ensure-that-a-partial-line-gets-displayed-p.patch, #46759
        StatusNone In Progress
        Assigned toNone bens
        SummaryUnindent occasionally causes underflow in update_softwrapped_line resulting in a freeze. unindent occasionally causes underflow in update_softwrapped_line() resulting in a freeze
    2019-04-10 dolorous Attached File- Added 0001-display-don-t-nibble-off-more-zero-width-characters-.patch, #46742
    2019-04-09 easyaspi314 Attached File- Added Perishable.cpp, #46735

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code