[bug] Segmentation fault in the "fc" builtin

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[bug] Segmentation fault in the "fc" builtin

Franklin, Jason
Greetings:

Yesterday, I encountered a segmentation fault when using the "fc"
builtin command.  I cloned the Bash source code from GNU Savannah, and I
verified that the bug is still present in the latest commits to the
master and devel branches (the work below applies to "devel").

To reproduce...

  $ bash --norc
  $ fc -0
  Segmentation fault (core dumped)

I worked with a colleague during our lunch break to track down the issue
with GDB.  We created a minimal patch (attached) that fixes the problem.

Allow me to explain the reasoning behind the patch...

From the CHANGES file, we see this note concerning the "fc" builtin:

  b.  The fc builtin now interprets -0 as the current command line.

This tells us the intention of the "-0" option, and, indeed, we can see
in the fc_gethnum() function that this intention is programmed in as we
would expect.  See the excerpt below.

   566      if (n < 0)
   567 {
   568  n += i + 1;
   569  return (n < 0 ? 0 : n);
   570 }
   571      else if (n == 0)
   572 return ((sign == -1) ? real_last : i);
   573      else
   574 {
   575  n -= history_base;
   576  return (i < n ? i : n);
   577 }

So, fc_gethnum() returns real_last when "-0" is passed in.  This is a
problem (solved in the patch) because the last history item (the current
command) is removed when editing so that hlist[real_last] is NULL.  The
segfault occurs at this call

   420      fprintf (stream, "%s\n", histline (i));

because "i" is real_last, which has been removed.

Our solution does not remove the last history item when the user passes
"-0" to tell "fc" to include it in the history and the list to edit.

Note that we don't make any sweeping changes to the code, we simply
avoid the segfault.  This is because the intent of this option isn't
documented officially in the "help" output, so we don't want to make any
assumptions beyond what is already in the code.

There are some edge cases that could be addressed and some regions of
code that could be refactored to improve the robustness of "fc", but the
main priority in our eyes was fixing the segfault.  It would for
example, be nice to add a test to prove that the problem remains fixed
into the future.

I worked in tandem with my colleague, Brandon Pfeifer, to track down and
fix this issue.  He deserves equal credit.  If you decide to include the
patch, please credit us in your changelog as report and patch by Jason
Franklin <[hidden email]> and Brandon Pfeifer
<[hidden email]>.

Thanks in advance for considering this change!

--
Jason Franklin



fc_fix.patch (780 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [bug] Segmentation fault in the "fc" builtin

Chet Ramey
On 5/5/20 9:21 AM, Franklin, Jason wrote:

> Greetings:
>
> Yesterday, I encountered a segmentation fault when using the "fc"
> builtin command.  I cloned the Bash source code from GNU Savannah, and I
> verified that the bug is still present in the latest commits to the
> master and devel branches (the work below applies to "devel").
>
> To reproduce...
>
>   $ bash --norc
>   $ fc -0
>   Segmentation fault (core dumped)
>
> I worked with a colleague during our lunch break to track down the issue
> with GDB.  We created a minimal patch (attached) that fixes the problem.

Thanks for the report and your careful analysis.

>
> Allow me to explain the reasoning behind the patch...
>
> From the CHANGES file, we see this note concerning the "fc" builtin:
>
>   b.  The fc builtin now interprets -0 as the current command line.
Yes, this is from one of the bash-4.3 testing releases. It's in response
to this message:

https://lists.gnu.org/archive/html/bug-bash/2013-08/msg00037.html

and deliberately works only for -l.

The question is what to do about the cases where -l isn't supplied, as
you observed. Dumping core is definitely the worst of the options.

> Our solution does not remove the last history item when the user passes
> "-0" to tell "fc" to include it in the history and the list to edit.

The issue I have with this solution is that it leads to an infinite loop
if the user doesn't change the command in the editor. If you use `fc -s -0'
the shell runs fc recursively until it runs out of stack space and then
dumps core.

You could easily say that this falls into the category of user error, and
I wouldn't argue, but as you also observe, there's nothing in the man page
prohibiting or even warning against it.

I'm leaning towards making 0 and -0 out-of-range errors for the non-listing
case. This is what other shells do (the netbsd and freebsd shells being
notable exceptions).

Chet
--
``The lyf so short, the craft so long to lerne.'' - Chaucer
                 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, UTech, CWRU    [hidden email]    http://tiswww.cwru.edu/~chet/

Reply | Threaded
Open this post in threaded view
|

Re: [bug] Segmentation fault in the "fc" builtin

Franklin, Jason
On 5/5/20 11:41 AM, Chet Ramey wrote:

> Thanks for the report and your careful analysis.
>
>>
>> Allow me to explain the reasoning behind the patch...
>>
>> From the CHANGES file, we see this note concerning the "fc" builtin:
>>
>>   b.  The fc builtin now interprets -0 as the current command line.
> Yes, this is from one of the bash-4.3 testing releases. It's in response
> to this message:
>
> https://lists.gnu.org/archive/html/bug-bash/2013-08/msg00037.html
>
> and deliberately works only for -l.
>
> The question is what to do about the cases where -l isn't supplied, as
> you observed. Dumping core is definitely the worst of the options.
>
>> Our solution does not remove the last history item when the user passes
>> "-0" to tell "fc" to include it in the history and the list to edit.
>
> The issue I have with this solution is that it leads to an infinite loop
> if the user doesn't change the command in the editor. If you use `fc -s -0'
> the shell runs fc recursively until it runs out of stack space and then
> dumps core.

You're right here, I think.  With this, it still seems easy to shoot
yourself in the foot.  As you say below, though, it might be reasonable
to call this a user error.  Though the command does offer this negative
possibility, it is logically consistent with the intent.

> You could easily say that this falls into the category of user error, and
> I wouldn't argue, but as you also observe, there's nothing in the man page
> prohibiting or even warning against it.

Agreed.  This is an undocumented feature, which is why Brandon and I had
a bit of trouble figuring out what "should" happen. :/

> I'm leaning towards making 0 and -0 out-of-range errors for the non-listing
> case. This is what other shells do (the netbsd and freebsd shells being
> notable exceptions).

Well, I think 0 and -0 have different intentions as it stands.
Currently, "0" indicates the command right before the "fc" invocation
that caused the editing or listing.  This shouldn't ever cause an
infinite loop and should not be an out-of-range error, I assert.

Example session:

  bash-5.0$ true # example command
  bash-5.0$ fc -l 0
  48       true # example command
  bash-5.0$

Thus, the argument in question is specifically "-0" proper.  This, to
me, means "the fc command itself" that did this work.

Would a good solution be to have "0" function as-is, but have "-0" only
be valid in the listing case?  This would avoid the problem above.

Of course, documenting the intent of the feature would be key to making
the change a successful one!

Thanks, Chet!

--
Jason Franklin

Reply | Threaded
Open this post in threaded view
|

Re: [bug] Segmentation fault in the "fc" builtin

Chet Ramey
On 5/5/20 12:16 PM, Franklin, Jason wrote:

> Agreed.  This is an undocumented feature, which is why Brandon and I had
> a bit of trouble figuring out what "should" happen. :/
>
>> I'm leaning towards making 0 and -0 out-of-range errors for the non-listing
>> case. This is what other shells do (the netbsd and freebsd shells being
>> notable exceptions).
>
> Well, I think 0 and -0 have different intentions as it stands.

In bash, yes. Nowhere else. They both require special handling.

> Currently, "0" indicates the command right before the "fc" invocation
> that caused the editing or listing.  This shouldn't ever cause an
> infinite loop and should not be an out-of-range error, I assert.

Yes, it's equivalent to -1. That's just giving it semantics, not providing
any unique functionality. It could just as easily have been an error, as
POSIX intended.

> Example session:
>
>   bash-5.0$ true # example command
>   bash-5.0$ fc -l 0
>   48       true # example command
>   bash-5.0$
>
> Thus, the argument in question is specifically "-0" proper.  This, to
> me, means "the fc command itself" that did this work.
>
> Would a good solution be to have "0" function as-is, but have "-0" only
> be valid in the listing case?  This would avoid the problem above.

This is about the only reasonable alternative.

> Of course, documenting the intent of the feature would be key to making
> the change a successful one!

I'll come up with something.

Chet

--
``The lyf so short, the craft so long to lerne.'' - Chaucer
                 ``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, UTech, CWRU    [hidden email]    http://tiswww.cwru.edu/~chet/