[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 <jason.franklin@quoininc.com> and Brandon Pfeifer
<brandon.pfeifer@quoininc.com>.

Thanks in advance for considering this change!

-- 
Jason Franklin

fc_fix.patch
Description: Text Data