Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove saving prior trap in PROMPT_COMMAND with subshell #60

Merged
merged 1 commit into from
Oct 19, 2017

Conversation

rcaloras
Copy link
Owner

  • This was causing crashes in cases where users were doing:
    export PROMPT_COMMAND="history -a; $PROMPT_COMMAND;"

  • Original description from contributor:
    $(trap -p DEBUG)" is launching a sub-shell, bash itself. So when people do
    "export PROMPT_COMMAND="history -a; $PROMPT_COMMAND;"", they export form the
    parent shell to subsequent sub-shells. And that creates the infinite loop,
    that eventually segfault bash.

- This was causing crashes in cases where users were doing:
  export PROMPT_COMMAND="history -a; $PROMPT_COMMAND;"

- Original description from contributor:
  $(trap -p DEBUG)" is launching a sub-shell, bash itself. So when people do
  "export PROMPT_COMMAND="history -a; $PROMPT_COMMAND;"", they export form the
  parent shell to subsequent sub-shells. And that creates the infinite loop,
  that eventually segfault bash.
@rcaloras rcaloras merged commit 850de53 into master Oct 19, 2017
@jombooth
Copy link
Contributor

LGTM

@rcaloras
Copy link
Owner Author

@jombooth 👍

@dimo414
Copy link
Collaborator

dimo414 commented Oct 20, 2017

I'm surprised that moving __bp_trap_string="$(trap -p DEBUG)" out of the PROMPT_COMMAND doesn't break trap preservation. Did you verify this is still working? It's possible the existing tests don't cover this sort of refactoring well enough.

@rcaloras
Copy link
Owner Author

@dimo414 You're right that this has broken trap preservation, sorry to have disrupted this feature. I wanted to put this through to remove the bug from our thread with Loic. If you have a way to still preserve it, please submit a PR 👍

@dimo414
Copy link
Collaborator

dimo414 commented Oct 21, 2017

That's fine, the patch is just confusing since it leaves bits of the trap-preservation logic in place (e.g. there's no point setting __bp_trap_string at all).

I'll file a separate bug to try to restore it.

lguelorget added a commit to lguelorget/bash-preexec that referenced this pull request Oct 24, 2017
- Avoids infinite recursion with things like export PROMPT_COMMAND="history -a; $PROMPT_COMMAND"
- Preserve trap DEBUG again: revert rcaloras#60
@lguelorget lguelorget mentioned this pull request Oct 24, 2017
rcaloras pushed a commit that referenced this pull request Oct 27, 2017
* Update bash-preexec.sh

- Avoids infinite recursion with things like export PROMPT_COMMAND="history -a; $PROMPT_COMMAND"
- Preserve trap DEBUG again: revert #60

* Update bash-preexec.sh

* Update bash-preexec.sh

* Update bash-preexec.sh

* Update bash-preexec.sh
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants