bugGNU nano - Bugs: bug #47544, do not call free on dirname result

 
 

bug #47544: do not call free on dirname result

Submitted by:  None
Submitted on:  Sat 26 Mar 2016 05:10:33 PM UTC  
 
Severity: 4 - ImportantStatus: 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 20 Jun 2016 09:50:04 AM UTC, comment #8:

The fix was release in 2.6.0.

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Mon 04 Apr 2016 07:27:11 PM UTC, comment #7:

Fixed in git, 33a6f6a1.

Thanks for reporting, all.

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Sat 02 Apr 2016 09:22:09 AM UTC, comment #6:

Hello Matthew,

Thanks for the complete explanations. But... did you see/try the patch? (The last one, file #36791.) Does it fully address the issue?

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Fri 01 Apr 2016 11:01:50 PM UTC, comment #5:

Hello,

If I open more than one file in nano 2.5.3 on MacOS X, it gets into a weird
state in the file input management code only when opening >1 file using
absolute pathnames. Here is what happened:

1) On accessing the second file, troubl begins in has_valid_path, when we are
executing the compound expression "dirname(mallocstrcpy(NULL, filename));".

2) This compound operation sets a pointer "dest" to 0x100400100 in my case and
fills it with some data.

3) It calls dirname and assigns the result to parentdir. But this is not safe
according to the dirname manpage on MacOS X:

"The dirname() function returns a pointer to internal storage space allocated
on the first call that will be overwritten by subsequent calls. Other vendor
implementations of dirname() may modify the contents of the string passed to
dirname(); if portability is desired, this should be taken into account when
writing code which calls this function."

4) Due to the issue in item (3) two kinds of undefined behavior can happen:

a) if dirname() is implemented where the memory is internal storage, the later
free(parentdir) destroys someone else's storage without permission

b) if dirname() is implemented where the memory is modifying the source
string, then we could be allocating a region at a specific start pointer and
freeing from a different start pointer inside the region

5) On this OS, I believe we are hitting case (a):

mallocstrcpy(NULL, filename) returns 0x100400100 in an example case.

parentdir contains 0x101001200 which is pretty far away (internal storage most
likely).

6) At the end of the function free(parentdir) is called illegally.

7) The illegal free() crashes the program:

/* Verify that the containing directory of the given filename exists. */
bool has_valid_path(const char *filename)
{
char *parentdir;
struct stat parentinfo;
bool validity = FALSE;

if (strrchr(filename, '/') == NULL)
parentdir = mallocstrcpy(NULL, ".");
else
parentdir = dirname(mallocstrcpy(NULL, filename)); *** UNDEFINED BEHAVIOR HERE ***

if (stat(parentdir, &parentinfo) == -1) {
if (errno == ENOENT)
statusbar(_("Directory '%s' does not exist"), parentdir);
else
statusbar(_("Path '%s': %s"), parentdir, strerror(errno));
} else if (!S_ISDIR(parentinfo.st_mode)) {
statusbar(_("Path '%s' is not a directory"), parentdir);
} else {
if (access(parentdir, X_OK) == -1)
statusbar(_("Path '%s' is not accessible"), parentdir);
else
validity = TRUE;
}

free(parentdir); *** ILLEGAL FREE CRASHES HERE ***

if (!validity)
beep();

return validity;
}

Thanks,
Matthew Hall

Matthew Hall <mhcptg>
Tue 29 Mar 2016 11:00:30 AM UTC, comment #4:

Thanks for confirming.

Looking at the patch again... Now that we have a copy of the name, there is no need to produce a "." ourselves, we can let dirname() do that. New version of patch is attached.

Oh, now that I speak a NetBSD'er... Could you try the patch posted in
https://lists.gnu.org/archive/html/nano-devel/2016-01/msg00024.html
and see if the cursor stays in the edit window and does not get miscplaced on the statusbar? You could verify this with 'src/nano --constantshow NEWS' and moving the cursor around, and also by typing Alt+Up and Alt+Down (which would normally produce unbound escape sequences and thus show a message).

(file #36791)

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Tue 29 Mar 2016 05:13:48 AM UTC, comment #3:

Patch looks good on NetBSD, thanks!
Didn't realize it is meant to warn about directory editing.

Anonymous
Sun 27 Mar 2016 08:07:54 PM UTC, comment #2:

Ehm... the crash is an unrelated problem -- thanks for making me find that. :) Your bestest patch simply does not do right thing (warn the user that she's trying to edit a directory), not on Linux at least.

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Sun 27 Mar 2016 07:54:53 PM UTC, comment #1:

Thanks for reporting. However, the proposed fix does not work on Linux: nano crashes when trying to edit a directory. For example: 'src/nano doc/'.

Attached patch works on Linux. Can you confirm that it works on NetBSD too?

(file #36781)

Benno Schulenberg <bens>
Project AdministratorIn charge of this item.
Sat 26 Mar 2016 05:10:33 PM UTC, original submission:

This causes a segfault in NetBSD-7.99.26, see http://gnats.netbsd.org/51010

From Linux dirname(3):

Both dirname() and basename() return pointers to null-terminated strings. (Do not pass these pointers to free(3).)

Anonymous

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #36791:  namecopy-condensed.patch added by bens (869B - text/x-diff)
file #36781:  namecopy.patch added by bens (827B - text/x-diff)
file #36777:  patch-src_files.c added by None (469B - text/x-csrc - Bestest patch)
file #36771:  patch2 added by None (398B - application/octet-stream - Better patch)
file #36769:  patch-src_files.c added by None (295B - text/x-csrc)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by mhcptg (Posted a comment)
  • -unavailable- added by bens (Updated 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 11 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 20 Jun 2016 09:50:04 AM UTCbensOpen/ClosedOpen=>Closed
    Mon 04 Apr 2016 07:27:11 PM UTCbensStatusNeed Info=>Fixed
    Tue 29 Mar 2016 05:59:46 PM UTCbensStatusNone=>Need Info
    Tue 29 Mar 2016 11:00:30 AM UTCbensAttached File-=>Added namecopy-condensed.patch, #36791
    Sun 27 Mar 2016 07:54:53 PM UTCbensAttached File-=>Added namecopy.patch, #36781
      Severity3 - Normal=>4 - Important
      Assigned toNone=>bens
      SummaryDo not call free on dirname result=>do not call free on dirname result
    Sun 27 Mar 2016 05:14:31 AM UTCNoneAttached File-=>Added patch-src_files.c, #36777
    Sat 26 Mar 2016 06:56:05 PM UTCNoneAttached File-=>Added patch2, #36771
    Sat 26 Mar 2016 05:10:33 PM UTCNoneAttached File-=>Added patch-src_files.c, #36769

    Back to the top


    Powered by Savane 3.1-cleanup1