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

 
 

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

bug #47544: do not call free on dirname result

Submitter:  None
Submitted:  Sat 26 Mar 2016 05:10:33 PM UTC
   
 
Severity:  4 - Important Status:  Fixed
Assigned to:  bens Open/Closed:  Closed

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>
Group administrator
Mon 04 Apr 2016 07:27:11 PM UTC, comment #7: 

Fixed in git, 33a6f6a1.

Thanks for reporting, all.

Benno Schulenberg <bens>
Group administrator
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>
Group administrator
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>
Group administrator
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>
Group administrator
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>
Group administrator
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.)

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

    Date Changed by Updated Field Previous Value => Replaced by
    2016-06-20 bens Open/ClosedOpen Closed
    2016-04-04 bens StatusNeed Info Fixed
    2016-03-29 bens StatusNone Need Info
    2016-03-29 bens Attached File- Added namecopy-condensed.patch, #36791
    2016-03-27 bens Attached 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
    2016-03-27 None Attached File- Added patch-src_files.c, #36777
    2016-03-26 None Attached File- Added patch2, #36771
    2016-03-26 None Attached File- Added patch-src_files.c, #36769

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code