Skip to content

ROX-32841: Quote args #295

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.

Sign up for GitHub

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

Draft
ovalenti wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

ROX-32841: Quote args #295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c0544f9
Escape process parameters
ovalenti Feb 16, 2026
c2765d8
Update tests to consider quoting
ovalenti Feb 16, 2026
bb74105
Formatting
ovalenti Feb 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ prost = "0.14.0"
prost-types = "0.14.0"
serde = { version = "1.0.219", features = ["derive"] }
serde_json = "1.0.142"
shlex = "1.3.0"
tokio = { version = "1.40.0", default-features = false, features = [
"fs",
"macros",
Expand Down
1 change: 1 addition & 0 deletions fact/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ prost = { workspace = true }
prost-types = { workspace = true }
serde = { workspace = true }
serde_json = { workspace = true }
shlex = { workspace = true }
uuid = { workspace = true }
yaml-rust2 = { workspace = true }

Expand Down
8 changes: 4 additions & 4 deletions fact/src/event/process.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,10 +192,10 @@ impl From<Process> for fact_api::ProcessSignal {

let container_id = container_id.unwrap_or("".to_string());

let args = args
.into_iter()
.reduce(|acc, i| acc + " " + &i)
.unwrap_or("".to_owned());
// try_join can fail if args contain nul bytes, though this should not happen
// since args are parsed from C strings which are nul-terminated
let args =
shlex::try_join(args.iter().map(|s| s.as_str())).unwrap_or_else(|_| String::new());
Comment on lines +197 to +198
Copy link
Contributor

@Molter73 Molter73 Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the parsing of arguments received from the kernel is done by searching for null characters, I don't think it is possible for shlex::try_join to fail here, because we've already removed them all. If that's the case, this might be better:

Suggested change
let args =
shlex::try_join(args.iter().map(|s| s.as_str())).unwrap_or_else(|_| String::new());
let Ok(args) = shlex::try_join(args.iter().map(|s| s.as_str())) else {
unreachable!();
};

Probably also want to update the comment to reflect this.


Self {
id: Uuid::new_v4().to_string(),
Expand Down
16 changes: 15 additions & 1 deletion tests/event.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import os
import re
from re import Pattern
import string
from enum import Enum
Expand All @@ -25,6 +26,19 @@ def extract_container_id(cgroup: str) -> str:
else:
return ''

def rust_style_quote(s):
if not s:
return "''"
if re.search(r'[^a-zA-Z0-9_./-]', s):
# Try to match the behavior of shlex.try_join()
if '\'' in s and not '"' in s:
return f'"{s}"'
escaped = s.replace("'", "\\'")
return f"'{escaped}'"
return s

def rust_style_join(args):
return ' '.join(rust_style_quote(arg) for arg in args)
Comment on lines +29 to +41
Copy link
Contributor

@Molter73 Molter73 Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried using Python's shlex and see if it behaves the same way as the Rust implementation? It would be a lot easier for tests.

Copy link
Contributor Author

@ovalenti ovalenti Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I used initially, and their quoting strategy is vastly different, unfortunately.

Copy link
Contributor Author

@ovalenti ovalenti Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, "vastly" was maybe a bit too strong : the "safe" characters list differ, in particular rust considers '=' and ':' unsafe.

This behavior is not tunable AFAICT, so I resorted to mimic things on the testing side.

Copy link
Contributor

@Molter73 Molter73 Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you put a comment detailing this so the next person doesn't try to use Python's shlex?


class EventType(Enum):
"""Enumeration for different types of file activity events."""
Expand Down Expand Up @@ -85,7 +99,7 @@ def get_id(line: str, wanted_id: str) -> int | None:
content = f.read(4096)
args = [arg.decode('utf-8')
for arg in content.split(b'\x00') if arg]
args = ' '.join(args)
args = rust_style_join(args)

with open(os.path.join(proc_dir, 'comm'), 'r') as f:
name = f.read().strip()
Expand Down
24 changes: 12 additions & 12 deletions tests/test_editors/test_nvim.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@

def test_new_file(editor_container, server):
fut = '/mounted/test.txt'
cmd = f"nvim {fut} '+:normal iThis is a test<CR>' -c x"

editor_container.exec_run(
f"nvim {fut} +':normal iThis is a test<CR>' -c x")
editor_container.exec_run(cmd)

process = Process.in_container(
exe_path='/usr/bin/nvim',
args=f'nvim {fut} +:normal iThis is a test<CR> -c x',
args=cmd,
name='nvim',
container_id=editor_container.id[:12],
)
Expand All @@ -25,12 +25,12 @@ def test_new_file(editor_container, server):

def test_open_file(editor_container, server):
fut = '/mounted/test.txt'
cmd = f"nvim {fut} '+:normal iThis is a test<CR>' -c x"
container_id = editor_container.id[:12]

# We ensure the file exists before editing.
editor_container.exec_run(f'touch {fut}')
editor_container.exec_run(
f"nvim {fut} +':normal iThis is a test<CR>' -c x")
editor_container.exec_run(cmd)

touch = Process.in_container(
exe_path='/usr/bin/touch',
Expand All @@ -40,7 +40,7 @@ def test_open_file(editor_container, server):
)
nvim = Process.in_container(
exe_path='/usr/bin/nvim',
args=f'nvim {fut} +:normal iThis is a test<CR> -c x',
args=cmd,
name='nvim',
container_id=container_id,
)
Expand Down Expand Up @@ -69,13 +69,13 @@ def test_open_file(editor_container, server):

def test_new_file_ovfs(editor_container, server):
fut = '/container-dir/test.txt'
cmd = f"nvim {fut} '+:normal iThis is a test<CR>' -c x"

editor_container.exec_run(
f"nvim {fut} +':normal iThis is a test<CR>' -c x")
editor_container.exec_run(cmd)

process = Process.in_container(
exe_path='/usr/bin/nvim',
args=f'nvim {fut} +:normal iThis is a test<CR> -c x',
args=cmd,
name='nvim',
container_id=editor_container.id[:12],
)
Expand All @@ -91,12 +91,12 @@ def test_new_file_ovfs(editor_container, server):

def test_open_file_ovfs(editor_container, server):
fut = '/container-dir/test.txt'
cmd = f"nvim {fut} '+:normal iThis is a test<CR>' -c x"
container_id = editor_container.id[:12]

# We ensure the file exists before editing.
editor_container.exec_run(f'touch {fut}')
editor_container.exec_run(
f"nvim {fut} +':normal iThis is a test<CR>' -c x")
editor_container.exec_run(cmd)

touch = Process.in_container(
exe_path='/usr/bin/touch',
Expand All @@ -106,7 +106,7 @@ def test_open_file_ovfs(editor_container, server):
)
nvim = Process.in_container(
exe_path='/usr/bin/nvim',
args=f'nvim {fut} +:normal iThis is a test<CR> -c x',
args=cmd,
name='nvim',
container_id=container_id,
)
Expand Down
20 changes: 12 additions & 8 deletions tests/test_editors/test_sed.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,22 @@
def test_sed(vi_container, server):
# File Under Test
fut = '/mounted/test.txt'
create_cmd = f"sh -c \"echo 'This is a test' > {fut}\""
sed_cmd = fr'sed -i -e "s/a test/not \\0/" {fut}'
container_id = vi_container.id[:12]

vi_container.exec_run(f"sh -c \"echo 'This is a test' > {fut}\"")
vi_container.exec_run(fr"sed -i -e 's/a test/not \0/' {fut}")
vi_container.exec_run(create_cmd)
vi_container.exec_run(sed_cmd)

shell = Process.in_container(
exe_path='/usr/bin/bash',
args=f"sh -c echo 'This is a test' > {fut}",
args=create_cmd,
name='sh',
container_id=container_id,
)
sed = Process.in_container(
exe_path='/usr/bin/sed',
args=fr'sed -i -e s/a test/not \0/ {fut}',
args=sed_cmd,
name='sed',
container_id=container_id,
)
Expand All @@ -40,20 +42,22 @@ def test_sed(vi_container, server):
def test_sed_ovfs(vi_container, server):
# File Under Test
fut = '/container-dir/test.txt'
create_cmd = f"sh -c \"echo 'This is a test' > {fut}\""
sed_cmd = fr'sed -i -e "s/a test/not \\0/" {fut}'
container_id = vi_container.id[:12]

vi_container.exec_run(f"sh -c \"echo 'This is a test' > {fut}\"")
vi_container.exec_run(fr"sed -i -e 's/a test/not \0/' {fut}")
vi_container.exec_run(create_cmd)
vi_container.exec_run(sed_cmd)

shell = Process.in_container(
exe_path='/usr/bin/bash',
args=f"sh -c echo 'This is a test' > {fut}",
args=create_cmd,
name='sh',
container_id=container_id,
)
sed = Process.in_container(
exe_path='/usr/bin/sed',
args=fr'sed -i -e s/a test/not \0/ {fut}',
args=sed_cmd,
name='sed',
container_id=container_id,
)
Expand Down
36 changes: 20 additions & 16 deletions tests/test_editors/test_vi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,15 @@ def test_new_file(vi_container, server):
fut = '/mounted/test.txt'
swap_file = '/mounted/.test.txt.swp'
swx_file = '/mounted/.test.txt.swx'
exe = '/usr/bin/vi'
exe = '/usr/libexec/vi'

vi_container.exec_run(
f"vi {fut} +':normal iThis is a test<CR>' -c x")
cmd = f"{exe} {fut} '+:normal iThis is a test<CR>' -c x"

vi_container.exec_run(cmd)

process = Process.in_container(
exe_path=exe,
args=f'vi {fut} +:normal iThis is a test<CR> -c x',
args=cmd,
name='vi',
container_id=vi_container.id[:12],
)
Expand Down Expand Up @@ -42,14 +43,15 @@ def test_new_file_ovfs(vi_container, server):
fut = '/container-dir/test.txt'
swap_file = '/container-dir/.test.txt.swp'
swx_file = '/container-dir/.test.txt.swx'
exe = '/usr/bin/vi'
exe = '/usr/libexec/vi'

cmd = f"{exe} {fut} '+:normal iThis is a test<CR>' -c x"

vi_container.exec_run(
f"vi {fut} +':normal iThis is a test<CR>' -c x")
vi_container.exec_run(cmd)

process = Process.in_container(
exe_path=exe,
args=f'vi {fut} +:normal iThis is a test<CR> -c x',
args=cmd,
name='vi',
container_id=vi_container.id[:12],
)
Expand Down Expand Up @@ -87,13 +89,14 @@ def test_open_file(vi_container, server):
swap_file = '/mounted/.test.txt.swp'
swx_file = '/mounted/.test.txt.swx'
vi_test_file = get_vi_test_file('/mounted')
exe = '/usr/bin/vi'
exe = '/usr/libexec/vi'
container_id = vi_container.id[:12]

cmd = f"{exe} {fut} '+:normal iThis is a test<CR>' -c x"

# We ensure the file exists before editing.
vi_container.exec_run(f'touch {fut}')
vi_container.exec_run(
f"vi {fut} +':normal iThis is a test<CR>' -c x")
vi_container.exec_run(cmd)

touch_process = Process.in_container(
exe_path='/usr/bin/touch',
Expand All @@ -103,7 +106,7 @@ def test_open_file(vi_container, server):
)
vi_process = Process.in_container(
exe_path=exe,
args=f'vi {fut} +:normal iThis is a test<CR> -c x',
args=cmd,
name='vi',
container_id=container_id,
)
Expand Down Expand Up @@ -147,13 +150,14 @@ def test_open_file_ovfs(vi_container, server):
swap_file = '/container-dir/.test.txt.swp'
swx_file = '/container-dir/.test.txt.swx'
vi_test_file = get_vi_test_file('/container-dir')
exe = '/usr/bin/vi'
exe = '/usr/libexec/vi'
container_id = vi_container.id[:12]

cmd = f"{exe} {fut} '+:normal iThis is a test<CR>' -c x"

# We ensure the file exists before editing.
vi_container.exec_run(f'touch {fut}')
vi_container.exec_run(
f"vi {fut} +':normal iThis is a test<CR>' -c x")
vi_container.exec_run(cmd)

touch_process = Process.in_container(
exe_path='/usr/bin/touch',
Expand All @@ -163,7 +167,7 @@ def test_open_file_ovfs(vi_container, server):
)
vi_process = Process.in_container(
exe_path=exe,
args=f'vi {fut} +:normal iThis is a test<CR> -c x',
args=cmd,
name='vi',
container_id=container_id,
)
Expand Down
28 changes: 16 additions & 12 deletions tests/test_editors/test_vim.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,13 @@ def test_new_file(editor_container, server):
swap_file = '/mounted/.test.txt.swp'
swx_file = '/mounted/.test.txt.swx'

editor_container.exec_run(
f"vim {fut} +':normal iThis is a test<CR>' -c x")
cmd = f"vim {fut} '+:normal iThis is a test<CR>' -c x"

editor_container.exec_run(cmd)

process = Process.in_container(
exe_path='/usr/bin/vim',
args=f'vim {fut} +:normal iThis is a test<CR> -c x',
args=cmd,
name='vim',
container_id=editor_container.id[:12],
)
Expand Down Expand Up @@ -41,12 +42,13 @@ def test_new_file_ovfs(editor_container, server):
swap_file = '/container-dir/.test.txt.swp'
swx_file = '/container-dir/.test.txt.swx'

editor_container.exec_run(
f"vim {fut} +':normal iThis is a test<CR>' -c x")
cmd = f"vim {fut} '+:normal iThis is a test<CR>' -c x"

editor_container.exec_run(cmd)

process = Process.in_container(
exe_path='/usr/bin/vim',
args=f'vim {fut} +:normal iThis is a test<CR> -c x',
args=cmd,
name='vim',
container_id=editor_container.id[:12],
)
Expand Down Expand Up @@ -85,10 +87,11 @@ def test_open_file(editor_container, server):
vi_test_file = get_vi_test_file('/mounted')
container_id = editor_container.id[:12]

cmd = f"vim {fut} '+:normal iThis is a test<CR>' -c x"

# We ensure the file exists before editing.
editor_container.exec_run(f'touch {fut}')
editor_container.exec_run(
f"vim {fut} +':normal iThis is a test<CR>' -c x")
editor_container.exec_run(cmd)

touch_process = Process.in_container(
exe_path='/usr/bin/touch',
Expand All @@ -98,7 +101,7 @@ def test_open_file(editor_container, server):
)
vi_process = Process.in_container(
exe_path='/usr/bin/vim',
args=f'vim {fut} +:normal iThis is a test<CR> -c x',
args=cmd,
name='vim',
container_id=container_id,
)
Expand Down Expand Up @@ -144,10 +147,11 @@ def test_open_file_ovfs(editor_container, server):
vi_test_file = get_vi_test_file('/container-dir')
container_id = editor_container.id[:12]

cmd = f"vim {fut} '+:normal iThis is a test<CR>' -c x"

# We ensure the file exists before editing.
editor_container.exec_run(f'touch {fut}')
editor_container.exec_run(
f"vim {fut} +':normal iThis is a test<CR>' -c x")
editor_container.exec_run(cmd)

touch_process = Process.in_container(
exe_path='/usr/bin/touch',
Expand All @@ -157,7 +161,7 @@ def test_open_file_ovfs(editor_container, server):
)
vi_process = Process.in_container(
exe_path='/usr/bin/vim',
args=f'vim {fut} +:normal iThis is a test<CR> -c x',
args=cmd,
name='vim',
container_id=container_id,
)
Expand Down
Loading