bugGNU roff - Bugs: bug #56694, [PATCH] troff mixes malloc /...

 
 

bug #56694: [PATCH] troff mixes malloc / delete[]

Submitter:  None
Submitted:  Mon 29 Jul 2019 09:17:29 PM UTC
   
 
Category:  Core Severity:  4 - Important
Item Group:  Crash/Unresponsive Status:  Fixed
Privacy:  Public Assigned to:  gbranden
Open/Closed:  Closed Planned Release:  1.23.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Tue 01 Mar 2022 02:08:10 PM UTC, comment #2: 


commit 06ee2524421ae607d0310204e3ddaea790af2fd6
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Thu Oct 22 21:03:09 2020 +1100

    Stop mixing up heap storage management strategies.

    ...in one respect, anyway.  A further audit would be good.

    Use only malloc() and free() to manage memory of paths opened by the
    parser instead of mixing in C++ new/delete management under some
    runtime-dependent circumstances.

    * src/libs/libgroff/relocate.cpp (relocatep): Use malloc(), not new.
    * src/roff/troff/input.cpp (process_macro_file, process_startup_file,
      macro_source): Use free(), not (a_)delete.

    Thanks to an anonymous contributor for the report and patch.

    Fixes <https://savannah.gnu.org/bugs/index.php?56694>.


G. Branden Robinson <gbranden>
Group administrator
Thu 22 Oct 2020 10:25:16 AM UTC, comment #1: 

Fixed in 06ee2524421ae607d0310204e3ddaea790af2fd6.

G. Branden Robinson <gbranden>
Group administrator
Mon 29 Jul 2019 09:17:29 PM UTC, original submission:  

I use Scudo [1] as the systemwide allocator on most of my systems. GNU troff is one of the very few binaries that fail with it, due to relying on undefined behavior with memory allocation.

Specifically, the search_path::open_file(..., char**) returns a path that is sometimes allocated with new[] but usually allocated with malloc (since that's how the "strsave" function is implemented -- it's basically an strdup). The returned paths are then deallocated using delete[], which is undefined behavior and only works with some lenient memory allocators.

Attached is a draft patch that solves the issue as far as I can tell with some basic testing, but I have no idea how common that pattern is in the codebase and I don't have a good way to test all of it. And it's not a beautiful patch either.

[1] https://llvm.org/docs/ScudoHardenedAllocator.html

Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #47276:  free.diff added by None (2KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by gbranden (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.

    Only logged-in users can vote.

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-10-22 gbranden StatusIn Progress Fixed
        Open/ClosedOpen Closed
        Planned ReleaseNone 1.23.0
    2020-10-21 gbranden Severity3 - Normal 4 - Important
        StatusNone In Progress
        Assigned toNone gbranden
    2020-10-13 barx Carbon-CopyRemoved 93119 -
    2020-10-13 barx Summarytroff mixes malloc / delete[] [PATCH] troff mixes malloc / delete[]
    2019-07-29 None Attached File- Added free.diff, #47276

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code