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

Crash if `hostname` is used as a loop variable #5548

Closed
acdha opened this Issue Jan 18, 2019 · 4 comments

Comments

Projects
None yet
3 participants
@acdha
Copy link

acdha commented Jan 18, 2019

I noticed an odd crash while running a command in a loop when I picked a loop variable which matched a variable name:

cadams@tethys ~> for hostname in (cat ~/Projects/…/django-hosts); curl --fail "http://$hostname/server-status?auto"; end
<E> fish: /tmp/fish-20181228-21895-1ih942f/fish-3.0.0/src/parse_execution.cpp:408: failed assertion: retval == ENV_OK && "for loop variable should have been successfully set"
<E> fish: Backtrace:
<E> fish: 0   parse_execution_context_t::run_for_statement(tnode_t<grammar::for_header>, tnode_t<grammar::job_list>) + 902
<E> fish: 1   parse_execution_context_t::run_block_statement(tnode_t<grammar::block_statement>, block_t const*) + 98
<E> fish: 2   parse_execution_context_t::run_1_job(tnode_t<grammar::job>, block_t const*) + 618
<E> fish: 3   parse_execution_context_t::run_job_conjunction(tnode_t<grammar::job_conjunction>, block_t const*) + 128
<E> fish: 4   parse_execution_result_t parse_execution_context_t::run_job_list<grammar::job_list>(tnode_t<grammar::job_list>, block_t const*) + 107
<E> fish: 5   parse_execution_context_t::eval_node(tnode_t<grammar::job_list>, block_t const*, io_chain_t const&) + 187
<E> fish: 6   int parser_t::eval_node<grammar::job_list>(std::__1::shared_ptr<parsed_source_t const>, tnode_t<grammar::job_list>, io_chain_t const&, block_type_t, std::__1::shared_ptr<job_t>) + 231
<E> fish: 7   parser_t::eval(std::__1::shared_ptr<parsed_source_t const>, io_chain_t const&, block_type_t) + 137
<E> fish: 8   parser_t::eval(std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> >, io_chain_t const&, block_type_t) + 162
<E> fish: 9   reader_run_command(parser_t&, std::__1::basic_string<wchar_t, std::__1::char_traits<wchar_t>, std::__1::allocator<wchar_t> > const&) + 419
<E> fish: 10  reader_read(int, io_chain_t const&) + 1376
<E> fish: 11  main + 4946
<E> fish: 12  start + 1

[Process completed]

This reproduces with other shell built-ins (e.g. umask or history).

Versions:

cadams@tethys ~> fish --version
fish, version 3.0.0
cadams@tethys ~> brew info fish
fish: stable 3.0.0 (bottled), HEAD
User-friendly command-line shell for UNIX-like operating systems
https://fishshell.com
/usr/local/Cellar/fish/3.0.0 (953 files, 8.3MB) *
  Poured from bottle on 2019-01-08 at 09:50:51
…
cadams@tethys ~> sw_vers
ProductName:	Mac OS X
ProductVersion:	10.14.2
BuildVersion:	18C54

@faho faho added the bug label Jan 18, 2019

@faho faho added this to the fish 3.0.1 milestone Jan 18, 2019

@faho

This comment has been minimized.

Copy link
Member

faho commented Jan 18, 2019

Indeed. The issue here is of course that $hostname is a read-only variable, so an assertion that "for loop variable should have been successfully set" fails.

@acdha

This comment has been minimized.

Copy link
Author

acdha commented Jan 18, 2019

I figured it was something like that since it was only pre-defined variables — thanks for the rapid response!

@faho faho closed this in 3847d2e Jan 18, 2019

@faho

This comment has been minimized.

Copy link
Member

faho commented Jan 18, 2019

Okay, this should be fixed now.

The issue turned out to be specifically for non-"electric" (i.e. evaluated when they are expanded, like $status) read-only variables like $hostname and $SHLVL.

Nice catch!

@ridiculousfish

This comment has been minimized.

Copy link
Member

ridiculousfish commented Jan 22, 2019

Picked into 3.0.1 as f2a1130

ridiculousfish added a commit that referenced this issue Jan 22, 2019

Also set the read-only flag for non-electric vars
For some reason, we have two places where a variable can be read-only:

- By key in env.cpp:is_read_only(), which is checked via set*

- By flag on the actual env_var_t, which is checked e.g. in
  parse_execution

The latter didn't happen for non-electric variables like hostname,
because they used the default constructor, because they were
constructed via operator[] (or some such C++-iness).

This caused for-loops to crash on an assert if they used a
non-electric read-only var like $hostname or $SHLVL.

Instead, we explicitly set the flag.

We might want to remove one of the two read-only checks, or something?

Fixes #5548.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.