bash - reproducible SEGFAULT on rapid SIGINTs
Hi, Sending rapid SIGINTs from another shell quickly results in a SEGFAULT of the bash instance receiving them. Steps to reproduce: * Open up a shell SHELL_1 and record its pid, e.g. "echo $$" * Launch another shell and send rapid SIGINTs: while true ; do kill -INT OTHER_PID; done SHELL_1 quickly dies with a SEGAULT Tested on GNU bash, version 5.2.32(1)-release (x86_64-pc-linux-gnu), compiled via plain ./configure && make Thanks and Kind Regards Tycho
Re: bash passes changed termios to backgrounded process(es) groups?
On 8/27/24 7:46 PM, Steffen Nurpmeso wrote:
Hello.
I got a bug report for my mailer which stated
$ ( echo blah | Mail root ) &
[1] 2754649
$ ^M^M^M^M^C^C
[1]+ Stopped ( echo blah | Mail root )
$ fg
( echo blah | Mail root )
$
I turns out i answered him now
The thing is that if i apply the patch (this to [master])
diff --git a/src/mx/termios.c b/src/mx/termios.c
index 733974ebce..08dd045226 100644
--- a/src/mx/termios.c
+++ b/src/mx/termios.c
@@ -152,6 +152,8 @@ a_termios_norm_query(void){
&a_termios_g.tiosg_normal->tiose_state) == 0);
/* XXX always set ECHO and ICANON in our "normal" canonical state */
a_termios_g.tiosg_normal->tiose_state.c_lflag |= ECHO | ICANON;
+ a_termios_g.tiosg_normal->tiose_state.c_iflag |= ICRNL;
+
/*NYD2_OU;*/
return rv;
}
then everything is working as should in an otherwise unchanged MUA.
It seems readline does this:
./lib/sh/shtty.c: ttp->c_iflag |= ICRNL; /* make sure we get CR->NL
on input */
./lib/readline/rltty.c: tiop->c_iflag &= ~(ICRNL | INLCR);
..and it seems that if bash starts a normal process then ICRNL is
set, but if it starts a (process)& or only process&, then not!
(I was about to send this to bug-readline first.)
Under no circumstances should a background process attempt to fetch or
modify terminal attributes. Why isn't your Mail process checking for that?
Chances are excellent that it will fetch the terminal attributes as they've
been set by readline when it goes to read the next command, then modify (?)
them out from underneath readline.
--
``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: Out of bounds read in parse.y.
On 8/27/24 3:58 PM, Collin Funk wrote:
I suspect there is a decrement that isn't matched by a call to
set_word_top(). But a reproducer would help, otherwise we're all just
guessing.
Sure, the bad read was happening while reading my .profile and .bashrc
file. I've narrowed it down to a bash completion file installed by my
system packages. I've attached it to this message.
Thanks. Here's the simple reproducer:
x()
{
case y in
z)
if (! false); then
foo=bar
fi
;;
esac
}
It was what I suspected.
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/
OpenPGP_signature.asc
Description: OpenPGP digital signature
Re: Surprising results when profiling Bash
On 8/24/24 1:46 PM, Martin D Kealey wrote: I've been making some tentative patches to the `devel` branch, and since I have a fairly large bashrc, when I compile Bash with maximal debugging support, its startup is ... underwhelmingly slothful. You're seeing the memory tracing and debugging code. So I decided to build it with profiling enabled, and see if I'd done something to ruin its performance. (Short answer: nope.) What stood out immediately is that 50%~90% of the time is spent in mregister_free(). In theory gprof separates the time spent in subordinate function calls, but there's no reporting of find_entry(), perhaps because it's 'static', and therefore in-lined, so perhaps that's the real culprit. What puzzles me is that this is much more than mregister_alloc(), during a phase when *most* of the activity is defining new stuff rather than getting rid of stuff. It's a simple table; alloc only has to find an empty slot, while free has to potentially search for a while to find the associated alloc. I haven't tweaked anything in this area of the code. Is this expected behaviour? Do I need to change my compilation options, or make any other changes? If you don't want it, compile without MALLOC_DEBUG defined. That's what release versions do. I haven't delved very deeply into this code, but it does seem to be preoccupied with managing signals, presumably because the code isn't re-entrant; so I'm wondering if it would be worthwhile to investigate different kinds of allocators, or perhaps a different approach to handling signals? It only blocks signals if it's running from a trap handler or SIGINT or SIGCHLD are trapped. Mostly legacy stuff from when SIGINT traps were handled immediately. -- ``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: Bash History Behavior Suggestion
On 8/20/24 1:42 AM, supp...@eggplantsd.com wrote: I know it can. The suggestion is that the default behavior needs some work: The default behavior provides mostly mechanism, not policy. There are ways to do what you want, but those are not suitable for (or desired by) everyone. So you can change your environment to suit your needs using the existing mechanisms. Extending the timestamp field in the history entry struct to contain more data after the timestamp has come up before, but I've never pursued it as a priority. -- ``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: Bash History Behavior Suggestion
On 8/20/24 2:20 PM, supp...@eggplantsd.com wrote: The problem with the tagged format is that it's *not* usable by grep Awk would have no problem with it. limited to exactly whatever magic is built into the "history" command That's where the magic should be. If that were the official interface to `.bash_history`, then bash has freedom to innovate on what is stored, and how it is stored. Then we could add even more information. The history file and history list are not bash-specific. The history library is part of readline and shared by many other applications. The history timestamp is managed through a history library interface, not via something bash does on its own. It's possible to extend the history APIs to deal with timestamps that have additional information after the timestamp, but it's not been a priority. -- ``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: bash passes changed termios to backgrounded process(es) groups?
Chet Ramey wrote in
<3ca901aa-5c5e-4be3-9a71-157d7101f...@case.edu>:
|On 8/27/24 7:46 PM, Steffen Nurpmeso wrote:
|> Hello.
|>
|> I got a bug report for my mailer which stated
|>
|> $ ( echo blah | Mail root ) &
|>[1] 2754649
|> $ ^M^M^M^M^C^C
|>
|>[1]+ Stopped ( echo blah | Mail root )
|> $ fg
|>( echo blah | Mail root )
|> $
|>
|> I turns out i answered him now
|>
|>The thing is that if i apply the patch (this to [master])
|>
|> diff --git a/src/mx/termios.c b/src/mx/termios.c
|> index 733974ebce..08dd045226 100644
|> --- a/src/mx/termios.c
|> +++ b/src/mx/termios.c
|> @@ -152,6 +152,8 @@ a_termios_norm_query(void){
|>&a_termios_g.tiosg_normal->tiose_state) == 0);
|> /* XXX always set ECHO and ICANON in our "normal" canonical \
|> state */
|> a_termios_g.tiosg_normal->tiose_state.c_lflag |= ECHO | ICANON;
|> + a_termios_g.tiosg_normal->tiose_state.c_iflag |= ICRNL;
|> +
|> /*NYD2_OU;*/
|> return rv;
|>}
|>
|>then everything is working as should in an otherwise unchanged MUA.
|>It seems readline does this:
|>
|> ./lib/sh/shtty.c: ttp->c_iflag |= ICRNL; /* make sure \
|> we get CR->NL on input */
|> ./lib/readline/rltty.c: tiop->c_iflag &= ~(ICRNL | INLCR);
|>
|> ..and it seems that if bash starts a normal process then ICRNL is
|> set, but if it starts a (process)& or only process&, then not!
|> (I was about to send this to bug-readline first.)
|
|Under no circumstances should a background process attempt to fetch or
|modify terminal attributes. Why isn't your Mail process checking for that?
How could it do so?
(getpid()==tcgetpgrp() or what the function name is is the only
idea i have, but note it is false for (EXE), too. *Big problem*!)
|Chances are excellent that it will fetch the terminal attributes as they've
|been set by readline when it goes to read the next command, then modify (?)
|them out from underneath readline.
Yes, it is not right what readline does there.
And for me, two things. For one we ensure we give to child
processes, and to restore whenever we go, the original settings as
we inherit them. We requery what means "original" whenever we get
back from a suspension, because user etc may apply changes, and we
should reflect (i have seen that, or even got a bug report).
And second, if there isatty(3) somewhere, we do termios stuff;
i agree this is bad, especially so since we look STDIN and STDOUT,
not STDIN and STDERR, as POSIX says a shell should do (i think
even dash that was notoriously "wrong" with that will have fixed
this with the next release). "Interactive" we are only if both
are isatty(3), and maybe i will change all that because we are no
shell and never will be, to only be interactive and do termios
stuff if all relevant FDs are terminals.
(Anyhow, in the example we start a child process, and since STDERR
is passed "as is", we try to restore termions settings (which,
btw, have never been changed in the above snippet, but that
aside), *if* i remember the context correctly. Our termios code
is a stack, and it is terribly complicated.)
--End of <3ca901aa-5c5e-4be3-9a71-157d7101f...@case.edu>
--steffen
|
|Der Kragenbaer,The moon bear,
|der holt sich munter he cheerfully and one by one
|einen nach dem anderen runter wa.ks himself off
|(By Robert Gernhardt)
Re: Out of bounds read in parse.y.
Chet Ramey writes:
> Thanks. Here's the simple reproducer:
>
> x()
> {
> case y in
> z)
> if (! false); then
> foo=bar
> fi
> ;;
> esac
> }
>
>
> It was what I suspected.
Ah, nice! Thank you for the help.
Collin
Re: Question on $@ vs $@$@
Date:Wed, 28 Aug 2024 02:03:54 +0200 From:Steffen Nurpmeso Message-ID: <20240828000354.qZaQvm7v@steffen%sdaoden.eu> | That confuses me again, unfortunately i got a bug report and | distracted. I mean, i would | | 1. skip leading whitespace anyhow (IFS or not, which |is a "documented bug" here i would say), |for the shell this would be: leading IFS whitespace, First, since you're concerned with your MUA, you can define whatever rules you like for this, there's nothing in any MUA spec I know of which requires anything like shell parsing/syntax/evaluation (with the possible exception of MH (nmh) and only because that actually uses the shell for everything other than the actual access to the messages, etc), so if you want, you get to do as you like. But if you're trying to emulate the shell rules, you should do it correctly, not just almost, or you'll confuse people. So above, "leading IFS whitespace" certainly. Further, if this reaches the end of the bytes subject to field splitting, you're done (this is the exit condition). | 2. pass by none-to-many non-IFS bytes, the "field data", then | | 3. |a. if there is a non-IFS-whitespace character: | - delimit the field, even with empty "field data", | |b. if there is a IFS-whitespace character: | - delimit the field only with non-empty "field data", No, you simply delimit the field. The field cannot be empty if the delimiter found is IFS whitespace, or you would have ignored that in the "skip leading IFS whitespace" above. That is, unless that #1 skip ran out of data (in which case you don't get here) it must have ended at either a non-IFS character (so the field is not empty, at the very least that character is in it) or a non_IFS-whitespace character (empty fields are allowed). | 4. skip trailing (new leading) (IFS-) whitespace Just "goto 1" (or "repeat"). The reason all this is messy, is that it is (more or less) the way it was implemented in the original Bourne shell. That tells you that the implementation must be simple - the rules might seem complex to explain, but the implementation is sure to be simple, because that shell wasted no code it could avoid. kre
Re: bash passes changed termios to backgrounded process(es) groups?
On Thu, 29 Aug 2024 at 06:12, Steffen Nurpmeso wrote: > Chet Ramey wrote in > <3ca901aa-5c5e-4be3-9a71-157d7101f...@case.edu>: > |On 8/27/24 7:46 PM, Steffen Nurpmeso wrote: > |> ..and it seems that if bash starts a normal process then ICRNL is > |> set, but if it starts a (process)& or only process&, then not! > |> (I was about to send this to bug-readline first.) > | > |Under no circumstances should a background process attempt to fetch or > |modify terminal attributes. Why isn't your Mail process checking for > that? > > How could it do so? > (getpid()==tcgetpgrp() or what the function name is is the only > idea i have, but note it is false for (EXE), too. *Big problem*!) > You'd want getpgid() or getpgrp(), rather than getpid(). (On Linux, getpgrp() returns the same as getpid() to the process group leader, but that's *not* true on *BSD & Darwin.) Having said that, it's more likely that Mail is actively doing something it shouldn't be doing, and stopping doing it will suffice to fix the issue. My first guess would be blocking or ignoring SIGTTIN and/or SIGTTOU. The default behaviour is for a background process to receive SIGTTIN or SIGTTOU when it attempts to interact with its controlling terminal. Indeed, not just the process itself, but every other process in the same process group too. And the default action in response to those signals is to stop, the same as SIGSTOP. SIGTTIN is always sent if you try to read from the tty, but SIGTTOU is only send after "stty tostop" or equivalent. So the main thing to do is to *avoid* ignoring or blocking those signals, and to remove "stty -tostop" from your ~/.profile (or to add "stty tostop" and complain to your OS vendor about their stupid default). My memory is a little hazy on what happens if you attempt tcgetattr() or tcsetattr() without otherwise reading or writing; in that case I suspect it doesn't send either signal right away; so perhaps then the fix is simply to try writing a welcome banner (or even just a single NL char) before attempting tcgetattr(). (Maybe a zero-sized read or write might suffice?) -Martin
Patch to unify shopt & set-o
Hi Chet On Wed, 28 Aug 2024 at 23:58, Chet Ramey wrote: > On 8/24/24 1:46 PM, Martin D Kealey wrote: > > I've been making some tentative patches to the `devel` branch, and since > I > > have a fairly large bashrc, when I compile Bash with maximal debugging > > support, its startup is ... underwhelmingly slothful. > > You're seeing the memory tracing and debugging code. > Thanks for that. The patch I was referring to is at https://github.com/kurahaupo/bash/tree/patch_options and it's almost ready to go; time to let some other eyeballs take a look at it. The purpose of my patch is to: 1. unify the handling of set -X, set -o XXX, and shopt -s XXX, so that either command can manipulate all options, and there's a single module underpinning both; 2. provide a pluggable framework, so that loadable modules can register new shopt/set -o tags (and de-register them before unloading the module); 3. gather all the logic for each option in one place, without forcing all options to be in one file. The code makes extensive use of designated initializers, so after merging, it would make C11 a requirement for building Bash. Please let me know if that's likely to be an issue. It has involved rewriting substantial portions of of flags.[ch], builtins/set.def, and builtins/shopt.def (around 60% of each file), and has created options.[ch] and examples/loadables/is_prime.c; the total lines changed is $( git diff -w -U0 devel..@ | wc -l ) == 5183 so I'm open to making stylistic and other adjustments if that would make it easier to merge. It passes all the manual tests I've thrown at it, but "make test" is still rather noisy so I have a way to go yet. In addition to the original goals, it now includes: * an example loadable that defines several options. * an additional help mechanism, with new text, so that "shopt --long-help OPTNAME" is more informative. * a much-simplified implementation of the "compat" options, since all options can now have computed values rather than *having* to set individual flag variables. * a clean up of anomalous whitespace (mostly because I kept tripping over it every time I tried to commit with my default "paranoid" checking enabled). * re-indenting a few small patches, to match what appears to be your preferred style. * some small tweaks to quell compiler warnings. * adjustments to xmalloc.[ch] to enable easier handling of "const" pointers; among other things, xxfree is a clone of xfree, except that it takes a const void* parameter instead of just void*. I've tried to sequence the commits so that they tell a clean narrative; all the whitespace changes are first, one commit per file, so that you can go "git diff -w devel..ws" to reassure yourself that no other changes have snuck in. Then the creation of the new "options" module; then integrating it with others; then removing old stuff that's no longer required; and finally moving the option definitions to be adjacent to the code whose behaviour is adjusted by each option. The last dozen or so commits are somewhat experimental, as I've adjusted the documentation framework somewhat, so I will probably continue to clean up and occasionally push-rebase my repo on github; please let me know when you'd like me to *stop* doing so, so that you can grab a branch for merging. I hope that helps your evaluation. Are there any other administrative points I need to address? -Martin
