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:  Confirmed
Privacy:  Public Assigned to:  None
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 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 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 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    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.5