bugGNU nano - Bugs: bug #52182, on Haiku and Gentoo, the first...

 
 

bug #52182: on Haiku and Gentoo, the first comments are uncolored at startup

Submitted by:  Benno Schulenberg <bens>
Submitted on:  Sat 07 Oct 2017 01:37:44 PM UTC  
 
Severity: 3 - NormalStatus: Fixed
Assigned to: Benno Schulenberg <bens>Open/Closed: Closed

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Mon 13 Nov 2017 07:06:01 PM UTC, comment #16:

Fixed in git, e6a92b14.

I didn't expect any noticeable slowdown in screen painting, just a slight waste of CPU cycles by ncurses for doing a little bit extra.

Thanks for testing, and for the A_PROTECT idea.

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Sun 12 Nov 2017 11:16:18 PM UTC, comment #15:

That works on an unpatched Gentoo and syntax highlighting doesn't appear to be any slower. Which is good because if Gentoo applies the fix, it will still have the 20150808 date.

Brand Huntsman <brand>
Sun 12 Nov 2017 12:39:19 PM UTC, comment #14:

A partial fix is not good enough. And a partial fix that causes some breakage elsewhere is unacceptable.

Luckily it is possible to detect the patch level of ncurses, so that the A_PROTECT hack is included only for the versions that are affected. Please check the attached patch, and verify that it fixes the problem on Gentoo when nano is linked against ncurses-6.0.

(You could also check out the aprotect branch from git, for convenience.)

(file #42375)

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Sun 12 Nov 2017 06:53:55 AM UTC, comment #13:

I don't think a hack that slows down nano for everyone should be applied to fix an issue only experienced by a few. Detecting ncurses 6.0 before adding A_PROTECT to syntax colors would also be unacceptable since the largest distros have a patched ncurses 6.0.

Updating statusbar immediately is understandable, but the titlebar isn't drawn when loading large files so not having wrefresh wouldn't matter. Reverting the wrefresh in titlebar() to wnoutrefresh should fix syntax highlighting for single/first buffer and break custom titlebar color. I don't how to fully fix syntax highlighting unless it would be simple(efficient) to add A_PROTECT to the first color match in the edit window. If not, then a partial fix is better than no fix, even at the expense of breaking custom titlebar colors.

The normal titlebar uses A_REVERSE and changing it to non-bright custom colors removes the A_REVERSE, allowing the ncurses problem to surface. Adding A_PROTECT to just UI colors should fix the titlebar coloring. This would be an acceptable temporary solution, as long as the A_PROTECT was flagged to be removed after ncurses 6.1 is released.

Something like the attached file.

(file #42370)

Brand Huntsman <brand>
Sat 11 Nov 2017 04:28:35 PM UTC, comment #12:

Forgot to attach the patch. Now then:

(file #42368)

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Sat 11 Nov 2017 04:27:49 PM UTC, comment #11:

Changing the wrefresh() call at the end of statusline() is not an option, because it would prevent any feedback when nano is loading a huge file or when searching takes a long time.

Also, on Haiku, changing those two wrefresh() calls to wnoutrefresh() does not solve the miscoloring issue.

What does solve it is to add the A_PROTECT attribute to all colors -- not just for the interface but also for the syntaxes. I cannot see any adverse effect (although I imagine the ncurses routines will get a bit slower because they have to handle an extra attribute -- so it would be nice to limit this workaround to just ncurses-6.0; but that would be a separate patch).

Does the attached patch solve the miscoloring you see on Gentoo? If so, and if you agree with the patch, please sign it off.

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Wed 08 Nov 2017 08:37:08 PM UTC, comment #10:

Gentoo only applies security and other important patches to packages that don't release often. I am going to submit the patch to Gentoo since nano is slightly broken by it, but ncurses 6.1 might be released before it gets applied.

The 42339 patch down below added A_PROTECT and changed two wrefresh calls. The A_PROTECT hack is no longer needed, so remove the first hunk from the patch.

The two wnoutrefresh calls should mostly fix the Alpine linux titlebar issue and the syntax highlighting which Alpine linux would also have. Maybe the bug reporter could apply the patch and test. And it appears Alpine linux uses or has a 2017 patch to ncurses and might no longer have either issue.

Current nano on an unpatched ncurses 6.0 will have broken syntax highlighting, for which the only easy fix is disabling syntax highlighting, pressing ^L when opening files or switching buffers, or using only bright colors in syntaxes.

With the two wnoutrefresh calls syntax highlighting will work for all nano users and titlebar coloring will slightly break, not as much as in the Alpine linux report, because the second wnoutrefresh fixes it a bit. Users suffering this problem can disable titlebar coloring, which is much easier to live without than syntax highlighting.

Brand Huntsman <brand>
Wed 08 Nov 2017 07:01:26 PM UTC, comment #9:

The ncurses patch should be applied to Gentoo's ncurses, so that all Gentoo users get the fix.

The reason that I don't see the bug on a recent Xubuntu is this:

$ dpkg --status libncursesw5 | grep Version
Version: 6.0+20160625-1ubuntu1

And on my Manjaro system, ncurses gets update every two, three months or so. I've just checked, and there it's 6.0+20170902. It would be better if ncurses made more frequent releases.

If we change the wrefresh() and the end of titlebar(), Alpine Linux would get the miscoloring problem back (as long as they don't patch their ncurses, assuming that the miscoloring has the same cause). So I want to hold off on that. And I don't know which other wrefresh() you mean. When you provide a patch, it will be clearer.

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Wed 08 Nov 2017 12:37:44 AM UTC, comment #8:

No need for an ncurses bug report, it was fixed two years ago.

commit def73dda3feef55f48cc205a763c06ba1ea70e78
patch 20151017
+ remove an early-return from _nc_do_color, which can interfere with data needed by bkgd when ncurses is configured with extended colors (patch by Denis Tikhomirov).

Applying the attached patch to ncurses 6.0 fixes the problem in nano. Otherwise the bug will persist until ncurses 6.1 is released.

The two wrefresh calls should probably be reverted to wnoutrefresh because they internally invoke doupdate(), which ideally should only be called once after all wnoutrefresh calls.

(file #42345)

Brand Huntsman <brand>
Tue 07 Nov 2017 04:58:17 PM UTC, comment #7:

Ah! Good hunting. So... a bug report to ncurses would be in order, no?

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Tue 07 Nov 2017 08:42:25 AM UTC, comment #6:

This bug and the musl bug might only be a problem in ncurses 6.0. I linked nano to ncurses 5.9 and both issues went away, works fine with wrefresh or wnoutrefresh. And the A_PROTECT patch below works fine with 5.9 and 6.0, so that is still an option.

Brand Huntsman <brand>
Tue 07 Nov 2017 05:17:43 AM UTC, comment #5:

The issue with wnoutrefresh in titlebar() is not unique to musl, I experience it on Gentoo with glibc. Any non-bright titlebar colors cause rendering issues. Changing the wrefresh to wnoutrefresh at the bottom of winio.c:statusline() fixes the titlebar, until a buffer switch. The statusline() wnoutrefresh also fixes the syntax highlighting without needing the edit_refresh in prepare_for_display().

Your wattron/wattroff trick with A_BOLD before setting the titlebar's color does nothing. But using any attribute for the titlebar color fixes the problem. The attached patch adds an A_PROTECT attribute to all interface_color_pair[] values. It doesn't appear to be needed for syntax colors and adding it would only slow down syntax highlighting.

I don't know if any terminals support A_PROTECT or if they would render protected text differently. And I'm not sure if another obscure attribute would be better or if the hack should even be added to nano.

(file #42339)

Brand Huntsman <brand>
Sun 22 Oct 2017 07:43:32 PM UTC, comment #4:

The commits you mention are not the cause of the miscolorings, they just expose an underlying bug. I was going to put money on it being an initialization failure somewhere in regcomp, but see below.

But indeed, using brightcyan instead of cyan, or making the two changes you mentioned (changing wrefresh() to wnoutrefresh() at the end of titlebar(), and refresh_need = TRUE to edit_refresh() at the end of prepare_for_display()) avoid the miscoloring also on Haiku.

Hrm. How do we work around this bug? When we change back to wnoutrefresh(), we get back a miscoloring on musl (bug #50787). And when changing back to edit_refresh(), we get back the "stammer" from bug #50877.

Hmm... Maybe it's not a regcomp bug, maybe it's an ncurses bug. That makes more sense: the brightness of a color does not influence how a regex is matched, it just changes the wattron() calls... Hmm... Maybe we can tickle ncurses into doing the right thing by doing a wattron() with a bright color at the start of each line, and then immediately switching it off again?

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Sat 21 Oct 2017 10:30:57 PM UTC, comment #3:

Commit 3275c4e514d8bd634116cf57387af68cabe2ab68 is what broke syntax highlighting when switching buffers. Reverting edit_refresh() at bottom of files.c:prepare_for_display() fixes the issue in buffers. Reverting the commit in the other comment is also required.

Brand Huntsman <brand>
Sat 21 Oct 2017 10:41:55 AM UTC, comment #2:

Reverting wrefresh() only works for the first buffer, the problem still happens when switching buffers.

Brand Huntsman <brand>
Sat 21 Oct 2017 09:56:20 AM UTC, comment #1:

I have this problem on Gentoo Linux and commit b98545f4551937af0dfd6fd818df65161ffc10fe is the problem for me, but only for non-bright colors at head of file. Change cyan to brightcyan for ## comments in nanorc.nanorc and see if that fixes the issue on Haiku. Reverting wrefresh() at bottom of winio.c:titlebar() to wnoutrefresh() fixes the issue for me.

Brand Huntsman <brand>
Sat 07 Oct 2017 01:37:44 PM UTC, original submission:

To reproduce, make sure that your ~/.nanorc includes the nanorc.nanorc syntax file. Then run:

src/nano syntax/default.nanorc

Notice how the two comment lines at the top of the file are not colored. Press ^L. Now the two lines are colored.

It seems that somehow something in the regex matching is uninitialized when nano starts up. And it gets initialized as soon as some other regex (other than the first that matched in the file) gets exercised. Weird!

I don't know whether this is a bug in Haiku, or in the regcomp implementation, or in nano. Valgrind always complains when running it on nano on a GNU/Linux system and when including a syntax that uses 'icolor', but I can't find anything that nano is doing wrong. It seems to be a fault somewhere in regcomp.c. I've always thought that it was a false positive from valgrind, but after seeing this miscoloring on Haiku...

==2168== Conditional jump or move depends on uninitialised value(s)
==2168== at 0x536B783: re_compile_fastmap_iter.isra.24 (regcomp.c:328)
==2168== by 0x537AFF5: re_compile_fastmap (regcomp.c:280)
==2168== by 0x537B6BA: regcomp (regcomp.c:509)
==2168== by 0x12149E: nregcomp (in /usr/local/bin/nano)
==2168== by 0x121E32: parse_colors (in /usr/local/bin/nano)
==2168== by 0x122666: parse_rcfile (in /usr/local/bin/nano)
==2168== by 0x122DC5: parse_includes (in /usr/local/bin/nano)
==2168== by 0x1226CA: parse_rcfile (in /usr/local/bin/nano)
==2168== by 0x122E81: parse_one_nanorc (in /usr/local/bin/nano)
==2168== by 0x122F58: do_rcfiles (in /usr/local/bin/nano)
==2168== by 0x11F7A9: main (in /usr/local/bin/nano)
==2168== Uninitialised value was created by a heap allocation
==2168== at 0x4C2DB2F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==2168== by 0x53739F1: create_token_tree (regcomp.c:3750)
==2168== by 0x53739F1: create_tree (regcomp.c:3740)
==2168== by 0x53739F1: lower_subexp (regcomp.c:1341)
==2168== by 0x537A285: lower_subexps (regcomp.c:1306)
==2168== by 0x537A285: postorder (regcomp.c:1220)
==2168== by 0x537A285: analyze (regcomp.c:1171)
==2168== by 0x537A285: re_compile_internal (regcomp.c:791)
==2168== by 0x537B6A9: regcomp (regcomp.c:498)
==2168== by 0x12149E: nregcomp (in /usr/local/bin/nano)
==2168== by 0x121E32: parse_colors (in /usr/local/bin/nano)
==2168== by 0x122666: parse_rcfile (in /usr/local/bin/nano)
==2168== by 0x122DC5: parse_includes (in /usr/local/bin/nano)
==2168== by 0x1226CA: parse_rcfile (in /usr/local/bin/nano)
==2168== by 0x122E81: parse_one_nanorc (in /usr/local/bin/nano)
==2168== by 0x122F58: do_rcfiles (in /usr/local/bin/nano)
==2168== by 0x11F7A9: main (in /usr/local/bin/nano)

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.

 

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

Attach File(s):
   
   
Comment:
   

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by brand (Posted a comment)
  • -unavailable- added by bens (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 10 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sun 19 Nov 2017 08:16:25 AM UTCbensOpen/ClosedOpen=>Closed
    Mon 13 Nov 2017 07:06:01 PM UTCbensStatusIn Progress=>Fixed
    Sun 12 Nov 2017 12:39:19 PM UTCbensAttached File-=>Added 0001-painting-evade-an-ncurses-bug-by-adding-the-A_PROTEC.patch, #42375
    Sun 12 Nov 2017 06:53:55 AM UTCbrandAttached File-=>Added 0001-for-ncurses-6.0-before-patch-20151017.patch, #42370
    Sat 11 Nov 2017 04:28:35 PM UTCbensAttached File-=>Added 0001-painting-evade-an-ncurses-bug-by-adding-the-A_PROTEC.patch, #42368
    Sat 11 Nov 2017 04:27:49 PM UTCbensStatusNone=>In Progress
      Assigned toNone=>bens
      Summaryon Haiku, the first comments are uncolored at startup=>on Haiku and Gentoo, the first comments are uncolored at startup
    Wed 08 Nov 2017 12:37:44 AM UTCbrandAttached File-=>Added ncurses-6.0-remove-an-early-return-from_nc_do_color.patch, #42345
    Tue 07 Nov 2017 05:17:43 AM UTCbrandAttached File-=>Added nano--fix-titlebar-and-syntax-highlighting-mis-coloring.patch, #42339

    Back to the top


    Powered by Savane 3.1-cleanup1