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.frank...@quoininc.com> and Brandon Pfeifer
<brandon.pfei...@quoininc.com>.
Thanks in advance for considering this change!
--
Jason Franklin
diff --git a/builtins/fc.def b/builtins/fc.def
index 6951a687..ba7c47bf 100644
--- a/builtins/fc.def
+++ b/builtins/fc.def
@@ -354,8 +354,10 @@ fc_builtin (list)
}
/* "When not listing, the fc command that caused the editing shall not be
- entered into the history list." */
- if (listing == 0 && hist_last_line_added)
+ entered into the history list." However, if the user passed "-0", then
+ histend will have been set to real_last above. This means the user wants
+ to include the current command, so we do not remove it here. */
+ if (listing == 0 && hist_last_line_added && histend < real_last)
{
bash_delete_last_history ();
/* If we're editing a single command -- the last command in the
