CircleCI-Public / circleci-cli

Use CircleCI from the command line
https://circleci-public.github.io/circleci-cli/
MIT License
407 stars 232 forks source link

CircleCI adds two null bytes to every `run` command's STDIN #456

Open schneems opened 4 years ago

schneems commented 4 years ago

Do you want to request a feature or report a bug?

Bug

What is the current behavior?

Circle CI adds two null characters to the STDIN of every run command. Here's a repro: https://github.com/schneems/circleci_null_bytes_in_stdin

====>> ruby -e "require \"io/console\"; STDIN.raw!; puts STDIN.read_nonblock(1000).inspect" || exit 0
  #!/bin/bash -eo pipefail
ruby -e "require \"io/console\"; STDIN.raw!; puts STDIN.read_nonblock(1000).inspect" || exit 0
"\x00\x00"

What is the expected behavior?

Circle CI should not add any values to STDIN. The above command should trigger an IO::EAGAINWaitReadable error

Which version of the CLI and OS are you using? Did this work in previous versions?

$ circleci version
0.1.9321+308499e (homebrew)
---
CircleCI CLI Diagnostics
---
Debugger mode: false
Config found: /Users/rschneeman/.circleci/cli.yml
API host: https://circleci.com
API endpoint: graphql-unstable
Error: please set a token with 'circleci setup'
You can create a new personal API token here:
https://circleci.com/account/api

OS: Mac (catalina) 10.15.6 (19G73)

If you have any questions, feel free to ping us at @CircleCI-Public/x-team.

I opened an internal support ticket about this https://support.circleci.com/hc/requests/77474.

This is the same bug as https://github.com/CircleCI-Public/circleci-cli/issues/455, but this reproduces it without any external dependencies, this only uses components provided directly by CircleCI.

edmorley commented 3 years ago

This also affected us in heroku/stack-images/pull/196 (regression after we migrated from Travis CI to Circle CI due to this issue).

schneems commented 3 years ago

I opened up (another) support ticket with Circle to get them to hopefully at a bare minimum acknowledge this issue after being open for 9 months. My new internal ticket number is #90444

lewang commented 3 years ago

Hi @schneems . Commands are run in a fake PTY. To avoid programs that require user input hanging, we send PTY control signals to indicate EOT(end of stream). The best way to address this is to respect the control signal. The --no-tty option of the heroku cli might work.

schneems commented 3 years ago

Thanks for the update. It's helpful to know that this behavior is intentional and it sounds like it won't be changing any time soon. I'm curious what interactive things need to be run but people don't interact with them or don't specify the inputs. We're also in the "arbitrary command execution" business and I don't think I've seen a case like that. Most cases people specify inputs or use the CLIs with flags like apt-get -y

Regarding --no-tty. Previously we used interactive mode to drive expensive sessions programmatically. See this example https://github.com/schneems/repl_runner#use

ReplRunner.new(:rails_console, "heroku run rails console -a testapp").run do |repl|
  repl.run('a = 1 + 1')         {|result| assert_match '2', result }
  repl.run('"hello" + "world"') {|result| assert_match 'helloworld', result }
  repl.run("a * 'foo'")         {|result| assert_match 'foofoo', result}
end

It's a pattern we discourage as it was less than deterministic. Funny though, that code is essentially trying to solve the "interactive process stuck" issue you're describing. Though I never had a case where someone wanted to run without specifying an input.

The problem extends to other commands we shell out to (I believe). The only reason it doesn't affect the heroku auth command that we shell out to is that it's short-circuited by an env var. It's a very common pattern for our tools to wrap other CLIs and shell out to them even when it's not the Heroku CLI. For example we're using github.com/heroku/cutlass to wrap and shell out to pack. I shell out to bash scripts as a form of unit testing in other places and assert on their returns https://github.com/heroku/buildpacks-ruby/blob/9b8c6c43ca3bd53bb8f915aa0a17636898c1d2c6/spec/unit/bash_functions_spec.rb#L28-L31.

Ideally, if we could disable this feature, or flush the null bytes somehow before executing our process I think that would be a more "solid" way to handle this case. Then we could have some kind of team-wide or company-wide "add this magic incantation to your circle config to prevent lost hours of debugging" instead of trying to find and address it at every possible time we shell out to a CLI. Do you have any suggestions for short scripts that might read in STDIN from bash that we could throw before our "real script"?

$ <bash read/flush stdin> && rake test