secrets env cmd command causing segfaults when you try to CTRL + C on the subcommand on slow calls

[tegefaulkes]

Describe the bug

During usage recently we've been experiencing segfaults while using the pk secrets env command. I haven't experienced this myself yet so I don't have the exact steps that cause this.

To Reproduce

People need to add details when they experience it.

Expected behavior

Running polykey secrets env should not cause a segfault.

Screenshots

Platform (please complete the following information)

• Device: [e.g. iPhone6]
• OS: [e.g. iOS]
• Version [e.g. 22]

Additional context

Notify maintainers

@tegefaulkes

[linear]

ENG-429 `secrets env` command causing segfaults

[tegefaulkes]

I've created this to track the problem. @CMCDragonkai @brynblack if you run into it at all then please add some details about what happened and why.

[aryanjassal]

Does this still happen? This hasn't been noticed for the past 2 months in almost regular usage.

[tegefaulkes]

I've never seen it happen. @brynblack @CMCDragonkai Have you seen this happen recently?

[aryanjassal]

Now that we are also using secrets env when launching the development shell, we still haven't noticed this issue. This means the issue could be already fixed. I'll be closing it now, and if the issue is encountered again, this can be re-opened with more information.

[CMCDragonkai]

I saw it efore. This happens for when you run a subcommand under secrets env, so not seeing it while running secrets env is not sufficient for closing this issue. There needs to be more tests on subcommands, and specifically it occurs for when you try to CTRL + C the subcommand due to some long running call to secrets env cmd.

[aryanjassal]

Running a potentially long operation like du -h / in the command and immediately cancelling it does not throw a segfault, but I did run into a segfault if i let the command run.

...
e}\0\0\u{14}\0\0\0\0\0\0\0\0\0\0\0\0\0��\0\0word/webSettings.xmlPK\u{1}\u{2}-\0\u{14}\0\u{6}\0\u{8}\0\0\0!\0\u{8}�u�\u{12}\u{1}\0\0�\u{1}\0\0\u{18}\0\0\0\0\0\0\0\0\0\0\0\0\0��\0\0customXml/itemProps2.xmlPK\u{5}\u{6}\0\0\0\0!\0!\0�\u{8}\0\0d�\0\0\0\0"`: file name contained an unexpected NUL byte', library/std/src/env.rs:353:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
fatal runtime error: failed to initiate panic, error 5
zsh: IOT instruction (core dumped)  pk secrets env vault -- 'du -h /'

So this might be related. If this was the exact error you were getting, then this would be useful to reproduce the issue.

[tegefaulkes]

It ran into a null byte where it doesn't expect it. file name contained an unexpected NUL byte. is that the full output of the failure?

[aryanjassal]

No, but there was a lot of junk. I piped the whole output into a file and got this. I was trying to export a .docx file as an environment variable, which ran into this error. So, the error I was getting is not related to this issue.

errlog.txt

[CMCDragonkai]

Running a potentially long operation like du -h / in the command and immediately cancelling it does not throw a segfault, but I did run into a segfault if i let the command run.

...
e}\0\0\u{14}\0\0\0\0\0\0\0\0\0\0\0\0\0��\0\0word/webSettings.xmlPK\u{1}\u{2}-\0\u{14}\0\u{6}\0\u{8}\0\0\0!\0\u{8}�u�\u{12}\u{1}\0\0�\u{1}\0\0\u{18}\0\0\0\0\0\0\0\0\0\0\0\0\0��\0\0customXml/itemProps2.xmlPK\u{5}\u{6}\0\0\0\0!\0!\0�\u{8}\0\0d�\0\0\0\0"`: file name contained an unexpected NUL byte', library/std/src/env.rs:353:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
fatal runtime error: failed to initiate panic, error 5
zsh: IOT instruction (core dumped)  pk secrets env vault -- 'du -h /'

So this might be related. If this was the exact error you were getting, then this would be useful to reproduce the issue.

Definitely had to do with the rust code.

[aryanjassal]

...
\0\0\0!\0!\0�\u{8}\0\0d�\0\0\0\0"`: file name contained an unexpected NUL byte
stack backtrace:
   0: rust_begin_unwind
   1: core::panicking::panic_fmt
   2: std::env::set_var::{{closure}}
             at /build/rustc-1.83.0-src/library/std/src/env.rs:364:9
   3: core::result::Result<T,E>::unwrap_or_else
             at /build/rustc-1.83.0-src/library/core/src/result.rs:1458:23
   4: std::env::set_var
             at /build/rustc-1.83.0-src/library/std/src/env.rs:363:43
   5: exec::execvp
             at /home/runner/work/js-exec/js-exec/src/napi/lib.rs:53:5
   6: exec::__napi__execvp::{{closure}}::{{closure}}
             at /home/runner/work/js-exec/js-exec/src/napi/lib.rs:20:1
   7: napi::tokio_runtime::within_runtime_if_available
             at /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/napi-2.10.1/src/tokio_runtime.rs:69:3
   8: exec::__napi__execvp::{{closure}}
             at /home/runner/work/js-exec/js-exec/src/napi/lib.rs:20:1
   9: core::result::Result<T,E>::and_then
             at /build/rustc-1.83.0-src/library/core/src/result.rs:1348:22
  10: exec::__napi__execvp
             at /home/runner/work/js-exec/js-exec/src/napi/lib.rs:20:1
  11: _ZN6v8impl12_GLOBAL__N_123FunctionCallbackWrapper6InvokeERKN2v820FunctionCallbackInfoINS2_5ValueEEE
  12: _ZN2v88internal25FunctionCallbackArguments4CallENS0_15CallHandlerInfoE
  13: _ZN2v88internal12_GLOBAL__N_119HandleApiCallHelperILb0EEENS0_11MaybeHandleINS0_6ObjectEEEPNS0_7IsolateENS0_6HandleINS0_10HeapObjectEEENS8_INS0_20FunctionTemplateInfoEEENS8_IS4_EEPmi
  14: _ZN2v88internal21Builtin_HandleApiCallEiPmPNS0_7IsolateE
  15: Builtins_CEntry_Return1_ArgvOnStack_BuiltinExit
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
thread '<unnamed>' panicked at core/src/panicking.rs:221:5:
panic in a function that cannot unwind
stack backtrace:
   0:     0x7fff98d905b9 - <std::sys::backtrace::BacktraceLock::print::DisplayBacktrace as core::fmt::Display>::fmt::h2f5a6b58013db6e9
   1:     0x7fff98da51e3 - core::fmt::write::hfa96557f28429d69
   2:     0x7fff98d7e6d3 - std::io::Write::write_fmt::h37f3912886494215
   3:     0x7fff98d90403 - std::sys::backtrace::BacktraceLock::print::h82a3e77ee14f93d1
   4:     0x7fff98d83ed3 - std::panicking::default_hook::{{closure}}::h4089ac0fd21dfdaa
   5:     0x7fff98d83cb7 - std::panicking::default_hook::hde6b2b92b4b8a422
   6:     0x7fff98d84344 - std::panicking::rust_panic_with_hook::h04d84002b0e420ae
   7:     0x7fff98d90986 - std::panicking::begin_panic_handler::{{closure}}::hf0b93ce775256187
   8:     0x7fff98d907e9 - std::sys::backtrace::__rust_end_short_backtrace::h13db8456c52fd998
   9:     0x7fff98d83f6c - rust_begin_unwind
  10:     0x7fff98cc978d - core::panicking::panic_nounwind_fmt::hb0e640aeb48a1ef8
  11:     0x7fff98cc9822 - core::panicking::panic_nounwind::h748de3548a6142ec
  12:     0x7fff98cc9985 - core::panicking::panic_cannot_unwind::h63fdedee79346b5b
  13:     0x7fff98ccd5e4 - exec::__napi__execvp::hc64900061483e1c9
                               at /home/runner/work/js-exec/js-exec/src/napi/lib.rs:20:1
  14:           0xb7c30e - _ZN6v8impl12_GLOBAL__N_123FunctionCallbackWrapper6InvokeERKN2v820FunctionCallbackInfoINS2_5ValueEEE
  15:           0xe9caf2 - _ZN2v88internal25FunctionCallbackArguments4CallENS0_15CallHandlerInfoE
  16:           0xe9d0c8 - _ZN2v88internal12_GLOBAL__N_119HandleApiCallHelperILb0EEENS0_11MaybeHandleINS0_6ObjectEEEPNS0_7IsolateENS0_6HandleINS0_10HeapObjectEEENS8_INS0_20FunctionTemplateInfoEEENS8_IS4_EEPmi
  17:           0xe9d938 - _ZN2v88internal21Builtin_HandleApiCallEiPmPNS0_7IsolateE
  18:          0x18dddf6 - Builtins_CEntry_Return1_ArgvOnStack_BuiltinExit
thread caused non-unwinding panic. aborting.

I enabled rust backtrace and this is what I got. The failure originates from js-exec's execvp.

[aryanjassal]

https://github.com/MatrixAI/Polykey-CLI/blob/staging/src/secrets/CommandEnv.ts#L218-L227

This might be the same problem solved in this commit:

843fc17

The contents of the file are being treated like a string and not binary data. While it is usually correct to treat environment variables as strings, if they contain binary data, that might result in the program incorrectly parsing and breaking the string.

A quick patch would be to update that to wrap in a Buffer.from() statement and run tests. A longer-term solution would be MatrixAI/Polykey#822, where directly transmitting binary data should prevent this issue from happening in the first place.