bugGNU nano - Bugs: bug #55169, with an invalid quoting regex,...

 
 

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

bug #55169: with an invalid quoting regex, paragraph movement functions crash

Submitter:  David Lawrence Ramsey <dolorous>
Submitted:  Wed 05 Dec 2018 06:12:18 PM UTC
   
 
Severity:  3 - Normal Status:  Fixed
Assigned to:  dolorous Open/Closed:  Closed

Jump to the original submission

Tue 11 Dec 2018 06:54:30 PM UTC, comment #12: 

It was not intended as exaggeration.

Other than a check for invalid syntax, there should be no policing of what the user tries to put in solely based on what's commonly used.  Not doing the word boundary character replacement in that case because it's not common to use word boundary functions in the quoting string is effectively doing that.

As for whether what the user is trying to do with the quoting string makes sense or not, if the user puts in something nonsensical, that should be on them, and nano should at least be resilient enough to try to handle it as-is if it will only take a minimum of code to work around it: in this particular case, it's a one-line fix.  If it were much more than that, it would be a different matter.  And if the issue would cause a security problem or a major issue, then it would also be a different matter.  In short, if the user wants to shoot themselves in the foot, it shouldn't be our job to police it, barring major issues.

But enough philosophy.  You're right in that having gnulib handle this instead is a better idea.

David Lawrence Ramsey <dolorous>
Group Member
Tue 11 Dec 2018 06:49:28 PM UTC, comment #11: 

Bug #55207 is the relevant report about the fixbounds() thing.  (I had looked at this before, but it fell through the cracks: it didn't make it onto my todo list.)

Benno Schulenberg <bens>
Group administrator
Tue 11 Dec 2018 06:35:32 PM UTC, comment #10: 

Oh, come on, don't exaggerate.  There is a difference between 1) the user making a mistake and nano helpfully alerting the user to it (instead of crashing), and 2) the user trying something that doesn't make sense in the first place and it not having any effect (or throwing an error) on one of the BSDs or some other platform.

But never mind, I will be erasing the whole fixbounds() function.  The configure script should check that the system's regex functions understand GNU-style word bounds, and if not, pull in the gnulib regex module.  That is what we have gnulib for: so nano doesn't have to deal with platform differences.

Benno Schulenberg <bens>
Group administrator
Tue 11 Dec 2018 12:50:13 PM UTC, comment #9: 

I don't recall ever using word boundaries in regex searches at all, but that doesn't mean they wouldn't ever be used.  And I don't have access to anything running one of the BSDs, so it wouldn't come up in my case anyway.  I was thinking more of if someone models new code based on that regcomp() call, they'll inadvertently be introducing a bug down the line.

Also, if you're going to keep thinking that way, it doesn't really make sense to set the quoting regex to something invalid and then use the paragraph movement functions, so this bug report should never have been filed.  It doesn't matter what the user tries as a quoting string; assuming it's valid, regardless of what it is, it should work.

David Lawrence Ramsey <dolorous>
Group Member
Tue 11 Dec 2018 10:04:40 AM UTC, comment #8: 

Thanks.  First patch applied, commit 6e3b9ac0, followed by some reshuffling.

I'm holding off on the second patch, even though it is the obviously cautious thing to do.  Because: can you think of any quoting regex where it makes sense to have a \< or \> in it?

Benno Schulenberg <bens>
Group administrator
Mon 10 Dec 2018 08:43:33 PM UTC, comment #7: 

Attached new version 3, against git c0abcc6.

Nano now dies if an invalid regex is specified.  This is much simpler than previous versions.

Note that there are two bits taken from previous versions of the patch set.  First, in the error message, the bad quoting regex is surrounded by double quotes (since that's done in every other bad regex message).  Second, the second patch ensures that fixbounds() is called on the regex in case the user specified word boundaries in the quoting string (since that's done in every other regcomp() call; it's a problem waiting to happen otherwise).

(file #45618)

David Lawrence Ramsey <dolorous>
Group Member
Sun 09 Dec 2018 04:42:42 PM UTC, comment #6: 

Hmm...  This duplicates some ten lines of code, twice.  This is not acceptable.

The rcfile stuff does not validate backupdir nor operatingdir; they get validated just once, in the main routine after everything has been read.  The quoting regex should be handled the same way: when it is invalid, nano exits.  That is a behavior change, but a good one, I think.

Benno Schulenberg <bens>
Group administrator
Sat 08 Dec 2018 12:03:40 AM UTC, comment #5: 

There's one minor oversight in the last patch set: a small memory leak.  The routine in main() that handles the -Q/--quotestr option should free quotestr before exiting if quotestr is set to an invalid regular expression.  I'm not in front of my development machine, though, so I can't fix this yet.

David Lawrence Ramsey <dolorous>
Group Member
Thu 06 Dec 2018 02:58:05 AM UTC, comment #4: 

After more testing, attaching patch set version 2, still against the same git version.

With this version, if the user specifies a valid regex in the nanorc, and an invalid regex on the comment line, nano now properly restores the previously compiled regex from the nanorc.  Sorry for the mess.

(file #45588)

David Lawrence Ramsey <dolorous>
Group Member
Wed 05 Dec 2018 11:09:30 PM UTC, comment #3: 

Argh.  Try this patch set instead.

The only difference between it and the last one is that it properly compiles the quoting string when it's specified via nanorc, and properly states "Bad quoting string" rather than "Bad regex" in the error message.  (It shouldn't have been using nregcomp() in rcfile.c.)

(file #45587)

David Lawrence Ramsey <dolorous>
Group Member
Wed 05 Dec 2018 10:24:47 PM UTC, comment #2: 

The attached patch set against git 55537dc should do it; it was tricky, so some testing is probably warranted.

The first patch in it actually fixes the problem, and the remaining bits rearrange things to avoid unnecessarily recompiling the regex if we have to fall back to the default, and add a bit of needed documentation.

(file #45586)

David Lawrence Ramsey <dolorous>
Group Member
Wed 05 Dec 2018 08:02:16 PM UTC, comment #1: 

Wow!  That's an old bug!  Even 2.2.6 and 2.1.10 and 2.0.6 segfault with your recipe.

Yes, the quoting regex should be checked for validity in all cases, preferrably at startup.  (I looked at this once, but it couldn't be done with rcfile_error() because then the command-line option isn't caught.)  If you have time for making a patch...

Benno Schulenberg <bens>
Group administrator
Wed 05 Dec 2018 06:12:18 PM UTC, original submission:  

This can be reproduced as follows (assuming that nano is built with regular expression support):

1. Run:

./nano -I -Q "[" +9,1 ../README

2. Press <Ctrl+W Ctrl+W> to attempt to move to the beginning of the paragraph.  Nano crashes instead.

3. Run:

./nano -I -Q "[" +9,1 ../README

2. Press <Ctrl+W Ctrl+O> to attempt to move to the end of the paragraph.  Nano crashes instead.

Obviously, it shouldn't crash.  Also, shouldn't there be a warning about an invalid quoting regex in these cases, as there is when justifying?

David Lawrence Ramsey <dolorous>
Group Member

 

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

Attached Files
file #45618:  regex-cleanup-3.zip added by dolorous (2KiB - application/zip)
file #45588:  regex-cleanup-2.zip added by dolorous (7KiB - application/zip)
file #45587:  regex-cleanup.zip added by dolorous (6KiB - application/zip)
file #45586:  regex-cleanup.zip added by dolorous (6KiB - application/zip)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by bens (Posted a comment)
  • -email is unavailable- added by dolorous (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-03-25 bens Open/ClosedOpen Closed
    2018-12-11 bens StatusIn Progress Fixed
        Assigned toNone dolorous
    2018-12-10 dolorous Attached File- Added regex-cleanup-3.zip, #45618
    2018-12-09 bens StatusNone In Progress
        Summarywith invalid regex strings, paragraph movement functions crash nano with an invalid quoting regex, paragraph movement functions crash
    2018-12-06 dolorous Attached File- Added regex-cleanup-2.zip, #45588
    2018-12-05 dolorous Attached File- Added regex-cleanup.zip, #45587
    2018-12-05 dolorous Attached File- Added regex-cleanup.zip, #45586

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code