[bug #55475] Segmentation fault in relocatep - groff 1.22.3

[Ingo Schwarze]

Update of bug #55475 (project groff):

                Severity:              3 - Normal => 2 - Minor              
                  Status:                    None => Confirmed              

    _______________________________________________________

Follow-up 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.

    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?55475>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/