bugGNU troff - Bugs: bug #55475, Segmentation fault in relocatep -...

 
 

bug #55475: Segmentation fault in relocatep - groff 1.22.3

Submitted by:  Danny Milosavljevic <dannym>
Submitted on:  Mon 14 Jan 2019 09:49:51 PM UTC  
 
Category:  Preprocessor preconv Severity:  4 - Important
Item Group:  Crash/Not responding Status:  Need Info
Privacy:  Public Assigned to:  G. Branden Robinson <gbranden>
Open/Closed:  Open Planned Release:  None

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

Wed 06 Jan 2021 04:59:09 AM UTC, comment #5: 

Well, no one has stepped up to this, and I can't reproduce it, but I also can't stand the thought of groff 1.23.0 going out with this bug, so with considerable discomfort I took a stab at it.

I've pushed the following 3 commits to try and get at the issue.  A tar archive of the tree can be obtained from:

https://git.savannah.gnu.org/cgit/groff.git/commit/?id=2fabd352f4ccdb382acffb7705a129977a2768d3

Or I can easily prepare a distribution archive from any of these three points if that would help.

I need someone's help to confirm whether the issue has been resolved, or to observe the assertion failure!

commit 2fabd352f4ccdb382acffb7705a129977a2768d3 (HEAD -> master, origin/master, origin/HEAD)
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Wed Jan 6 15:48:39 2021 +1100

    src/libs/libgroff/relocate.cpp: Shift #ifdef.

    * src/libs/libgroff/relocate.cpp (set_current_prefix) [!_WIN32]: Move
      logic attempting to set `curr_prefix` by calling searchpathext() from
      here...
      [WIN32]: ...to here.  The PATHEXT environment variable has semantics
      only under Windows, not POSIX systems, so the placement of this code
      seemed erroneous.

commit c2f0e424e4a2bdcd287c8be9957daf93a581673a
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Wed Jan 6 15:43:57 2021 +1100

    src/libs/libgroff/relocate.cpp: Fix memory leak.

    * src/libs/libgroff/relocate.cpp (set_current_prefix) [_WIN32]: Allocate
      memory from heap for `curr_prefix` only on Windows; on other systems,
      this file's searchpath() is used to populate `curr_prefix`, and that
      function (except on Windows) performs its own allocation.  Fixes
      memory leak noted by Ingo Schwarze.

commit 89c98409d32d01867e6f7cb7ab61efaf7b1da67e
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Wed Jan 6 15:34:50 2021 +1100

    [libgroff]: (relocatep) Add assertion.

    * src/libs/libgroff/relocate.cpp (relocatep): Add assertion to identify
      logic error if `curr_prefix` is unexpectedly a null pointer.  See
      <https://savannah.gnu.org/bugs/?55475>.

G. Branden Robinson <gbranden>
Project AdministratorIn charge of this item.
Wed 16 Jan 2019 10:37:53 AM UTC, comment #4: 

Re comment #3:

> I suspect that argv[0] is either NULL or some funny string that
> has nothing to do with the executable (that's permissible and
> can be done by the invoker) and searchpath() then can't access
> or realpath() it.


Oh, good point.  Yes, i overlooked that.

A real-world crash is not "MINOR", so i changed the severity to "IMPORTANT".  Given the contorted code, i'm not sure which is the best fix, though, or what the point of all that code may be in the first place.  But maybe some other groff developer is more familiar with it.

Ingo Schwarze <schwarze>
Project Member
Wed 16 Jan 2019 10:05:40 AM UTC, comment #3: 

We hit the bug I described in production in the Guix distribution (100% reproducible without racing-deleting any executable or anything like that).  It's not a theoretical problem.  See also https://debbugs.gnu.org/cgi/bugreport.cgi?bug=30785 for the Guix bug report (it was a nightmare to find the cause).

I think (but have no direct evidence) that searchpath returns NULL because we see that curr_prefix ended up NULL (we saw that in a backtrace).  Since curr_prefix is only set to the allocation using "new char[...]" and then to the result of the searchpath() call, assuming we have enough RAM (we do), that leaves the searchpath() call as only possible culprit.

I suspect that argv[0] is either NULL or some funny string that has nothing to do with the executable (that's permissible and can be done by the invoker) and searchpath() then can't access or realpath() it.

realpath() can fail because of: search permission denied, path is NULL, I/O error, loop in symbolic links, name too long, named file does not exist (!), out of memory and if some non-leaf part of the path isn't a directory.

preconv is called by man via libpipeline and we don't have any experience what funny stuff it does.

Anyway, we will patch it locally by disabling the entire rube goldberg thing, substituting "-DENABLE_RELOCATABLE=1" by the empty string.  Then it works fine.

Danny Milosavljevic <dannym>
Wed 16 Jan 2019 06:18:56 AM UTC, comment #2: 

Bug set to "CONFIRMED".

I fail to see how searchpath() can actually return NULL in this context, given that it is called with an argument of program_name, which i don't think can ever be NULL, i don't see how access(F_OK) could possibly fail, and then realpath(3) is called on it, which cannot fail either, and the result returned.  Oh wait, yes, it can fail as a result of a race condition, if you start preconv and than delete the binary file from the disk before the program gets far enough for the access(2) or the realpath(3) call.

That said, the function set_current_prefix() obviously assumes that searchpath() can return NULL because it tests !curr_prefix
and returns leaving curr_prefix == NULL it that case.  So the code is incorrect either way.  Either relocatep() needs to deal with the case of curr_prefix remaining NULL even after calling set_current_prefix(), or the unreachable code should be removed from its subroutines.

All the same, i reduced the severity to "MINOR" because it looks like the bug cannot actually be triggered in practice, except in insane edge cases like deleting the program exactly at the same time as starting it.

That said, all this code is obviously overengineered to the point of absurdity.  Someone should really go in there and delete most of it, leaving only what it is really needed.  I'm not committing a fix because that code is more than an order of magnitude too contorted for my taste, making me reluctant to touch it at all, and i'm not going to do the required cleanup myself because the code quality in these files is so abysmal that it would cause much more work than i'm willing to invest into these files.

By the way, there is at least one bug in the vicinity that is a lot more obvous than what you are reporting:  set_current_prefix() says, in two consecutive lines:

curr_prefix = new char[path_name_max()];
curr_prefix = searchpath(program_name, getenv("PATH"));

That's an obvious and unconditional memory leak.

Ingo Schwarze <schwarze>
Project Member
Tue 15 Jan 2019 11:38:33 AM UTC, comment #1: 

Furthermore, "relocate" is apparently only used in your local copy of localcharset.c, which is unconditionally compiled and used even though all glibc > 2.1 already contain a similar version (without the crash bug).  It might make sense to conditionally compile localcharset.c .

Danny Milosavljevic <dannym>
Mon 14 Jan 2019 09:49:51 PM UTC, original submission:  

relocatep segfaults when libpipeline invokes preconv in a way that preconv can't find itself in $PATH (and argv[0] not being an absolute path either).

Reason:

libs/libgroff/relocate.cpp relocatep uses curr_prefix as a source for memcpy, even though set_current_prefix could end up setting it to 0 (for example when searchpath failed to find a file whose basename is equal to argv[0] in $PATH).

That causes preconv to segfault in the case above.

Fix:

Add a "configure" option to disable -DENABLE_RELOCATABLE .

It's not customary to have relocatable things like that in UNIX--and with the advent of namespaces, I suggest not to do it in the case of UNIX at all (what for?).

Danny Milosavljevic <dannym>

 

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

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by gbranden (Posted a comment)
  • -email is unavailable- added by schwarze (Posted a comment)
  • -email is unavailable- added by dannym (Submitted the item)
  • -email is unavailable- added by dannym
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-01-06 gbranden StatusConfirmed => Need Info
        Assigned toNone => gbranden
    2019-01-16 schwarze Severity2 - Minor => 4 - Important
    2019-01-16 schwarze Severity3 - Normal => 2 - Minor
        StatusNone => Confirmed
    2019-01-14 dannym Carbon-Copy- => Added dannym

    Back to the top


    Powered by Savane 3.7