bugGNU nano - Bugs: bug #51405, not defining all interface colors...

 
 

bug #51405: not defining all interface colors leads to a segfault

Submitted by:  David Lawrence Ramsey <dolorous>
Submitted on:  Thu 06 Jul 2017 07:40:45 PM UTC  
 
Severity: 5 - BlockerStatus: 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)

Fri 07 Jul 2017 10:00:17 AM UTC, comment #11:

Oh, and closing because not released.

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Fri 07 Jul 2017 09:58:30 AM UTC, comment #10:

Fixed in git, ac726f08.

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Fri 07 Jul 2017 08:24:48 AM UTC, comment #9:

Looks good to me. Sorry for the confusion.

David Lawrence Ramsey <dolorous>
Project Administrator
Fri 07 Jul 2017 08:01:06 AM UTC, comment #8:

Yes, after following my own recipe, I was able to reproduce the segfault. Before, I wasn't able because my /etc/nanorc contains colors for all five interface elements.

I too didn't realize that parse_color_names() is called also from colors.c, but I knew that rcfile.c could not possibly cause the segfault, because when parse_color_names() is called there, combostr cannot be NULL -- the string might be empty, but the pointer can never be NULL.

So I am going to fix the issue in a different way: preventing the call in color.c when the relevant element has not been defined. See attached patch.

(file #41146)

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Fri 07 Jul 2017 02:06:43 AM UTC, comment #7:

Okay, after more testing, it seems the segfault has nothing to do with syntaxes. I've been able to reproduce it with current git (1695463), and the error in unquoted.nanorc about having no color directives is displayed properly. The problem is still rooted in strchr()'s being passed NULL, but the circumstances are different.

parse_color_names() is called in color.c:set_colorpairs() to color parts of nano's interface. If any of the five commands that color nano's interface (set titlecolor, set statuscolor, set keycolor, set numbercolor, set functioncolor) are not explicitly in the nanorc, this will cause the segfault because one or more of the elements of specified_color_combo[] will be NULL, and parse_color_names() will try to parse all five elements regardless of that, with specified_color_combo[]'s being used as combostr.

David Lawrence Ramsey <dolorous>
Project Administrator
Fri 07 Jul 2017 12:09:38 AM UTC, comment #6:

...and when no syntaxes are loaded, that's with nano built only with "CFLAGS="-g3" ./configure && make", with a .nanorc consisting only of "set" and "unset" statements, so you can be sure no syntaxes are loaded in that case.

David Lawrence Ramsey <dolorous>
Project Administrator
Fri 07 Jul 2017 12:07:02 AM UTC, comment #5:

And for the record, my previous patch also fixes the segfault in the aforementioned case when no syntaxes are loaded, but that makes me wonder why the color names are being parsed at all if no syntaxes are loaded.

David Lawrence Ramsey <dolorous>
Project Administrator
Fri 07 Jul 2017 12:01:18 AM UTC, comment #4:

Also, after some more thought, the problem isn't even limited to the one syntax. If nano is compiled with "CFLAGS="-g3" ./configure && make", that leaves sysconfdir set to /usr/local/etc and shareddir set to /usr/local/share/nano. On my system, /usr/local/etc is empty, and /usr/local/share/nano doesn't exist, so when nano is compiled this way, it finds no syntaxes and loads none. And yet the segfault still occurs in this case: according to gdb, at the beginning of color.c:parse_color_names(), syntaxes is 0x0 (NULL) and combostr is 0x0 (NULL) at the time of the segfault.

David Lawrence Ramsey <dolorous>
Project Administrator
Thu 06 Jul 2017 10:39:38 PM UTC, comment #3:

For the record, I'm using Slackware 14.2 with all current security fixes applied (i.e., glibc 2.23 plus security fixes for it specifically). Also, after some googling, it seems that passing NULL to strchr() (or other C string functions, for that matter) leads to undefined behavior, and glibc specifically doesn't handle NULL in such cases.

http://pubs.opengroup.org/onlinepubs/9699919799/functions/strchr.html
http://en.cppreference.com/w/c/string/byte/strchr
https://bugzilla.redhat.com/show_bug.cgi?id=1018331

David Lawrence Ramsey <dolorous>
Project Administrator
Thu 06 Jul 2017 08:48:41 PM UTC, comment #2:

Done. Moved /etc/nanorc to /etc/nanorc.old, ~/.nanorc to ~/.nanorc.old, did a straight "CFLAGS="-g3" ./configure && make", and the segfault still occurs exactly as before.

David Lawrence Ramsey <dolorous>
Project Administrator
Thu 06 Jul 2017 08:21:37 PM UTC, comment #1:

I cannot reproduce. :|

Please configure nano without any options, move your /etc/nanorc out of the way, and make your ~/.nanorc consist of just:

syntax "unquoted"
comment """

Can you reproduce it then? If not, then start adding back slowly the options you normally have in your .nanorc and /etc/nanorc.

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Thu 06 Jul 2017 07:40:45 PM UTC, original submission:

As of current git (f46544f), nano no longer properly handles syntaxes with no color elements. This can be reproduced as follows:

1. Create a new syntax file in /usr/share/nano/unquoted.nanorc, containing:

syntax "unquoted"
comment """

2. Build nano from git with --sysconfdir=/etc and --prefix=/usr.

3. Run:

./nano -Y unquoted

4. After any non-color-related nanorc warnings, nano will segfault instead of showing an error that the syntax "unquoted" has no color commands. A backtrace shows that the segfault is in parse_color_names():778, the first line of parse_color_names(). Since the syntax has no colors, combostr is NULL, and the attempt to run strchr() on NULL causes the segfault.

The problem seems to have originated in commit 9462ba8, which, as part of simplifying color parsing, removed the NULL check for combostr. The attached crude patch puts it back into the new code and avoids the problem (no segfault, and the error regarding no color commands is back), although there might be a simpler way to do it.

David Lawrence Ramsey <dolorous>
Project Administrator

 

(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 bens (Posted a comment)
  • -unavailable- added by dolorous (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 8 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 07 Jul 2017 10:00:17 AM UTCbensOpen/ClosedOpen=>Closed
    Fri 07 Jul 2017 09:58:30 AM UTCbensStatusIn Progress=>Fixed
    Fri 07 Jul 2017 08:01:06 AM UTCbensAttached File-=>Added prevent-segfault-because-of-pristine-interface.patch, #41146
      Severity3 - Normal=>5 - Blocker
      StatusNone=>In Progress
      Assigned toNone=>bens
      Summaryparsing of syntaxes without colors leads to a segfault=>not defining all interface colors leads to a segfault
    Thu 06 Jul 2017 07:40:45 PM UTCdolorousAttached File-=>Added fix-color_names-null-check.patch, #41136

    Back to the top


    Powered by Savane 3.1-cleanup1