[bug] Segmentation fault in the "fc" builtin
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.
566if (n < 0)
567 {
568n += i + 1;
569return (n < 0 ? 0 : n);
570 }
571else if (n == 0)
572 return ((sign == -1) ? real_last : i);
573else
574 {
575n -= history_base;
576return (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
420fprintf (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 and Brandon Pfeifer
.
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
[bug] Segmentation fault in the "fc" builtin (additional change)
Greetings:
After sending the original email concerning this issue, I delved into
the code again for one last review. I discovered that, even after the
original fix a segmentation fault is still possible, because histbeg
could be higher than histend! The idea from "fc" would be to print the
list in reverse.
However, histbeg and histend are swapped after histend is checked in the
original patch. So, I have attached an updated patch that avoids the
following additional segfault:
$ bash --norc
$ fc -0 100
Segmentation fault (core dumped)
This patch works by moving the reversal of histbeg and histend to before
when histend is checked. This handles the other case when a segfault
could occur (where histbeg is real_last).
Thanks again for reviewing!
--
Jason Franklin
diff --git a/builtins/fc.def b/builtins/fc.def
index 6951a687..04361b92 100644
--- a/builtins/fc.def
+++ b/builtins/fc.def
@@ -353,9 +353,20 @@ fc_builtin (list)
histbeg = histend = last_hist;
}
+ if (histend < histbeg)
+{
+ i = histend;
+ histend = histbeg;
+ histbeg = i;
+
+ reverse = 1;
+}
+
/* "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
@@ -382,15 +393,6 @@ fc_builtin (list)
return (EXECUTION_FAILURE);
}
- if (histend < histbeg)
-{
- i = histend;
- histend = histbeg;
- histbeg = i;
-
- reverse = 1;
-}
-
if (listing)
stream = stdout;
else
Re: [bug] Segmentation fault in the "fc" builtin
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, CWRUc...@case.eduhttp://tiswww.cwru.edu/~chet/
Re: [bug] Segmentation fault in the "fc" builtin
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
Re: [bug] Segmentation fault in the "fc" builtin
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, CWRUc...@case.eduhttp://tiswww.cwru.edu/~chet/
