Skip to content

Fix bugs in argument passing to scripts#177

Merged
mplegendre merged 4 commits into
llnl:develfrom
mplegendre:bugfix/shell_parameters
May 13, 2026
Merged

Fix bugs in argument passing to scripts#177
mplegendre merged 4 commits into
llnl:develfrom
mplegendre:bugfix/shell_parameters

Conversation

@mplegendre
Copy link
Copy Markdown
Member

We had some failures in an application that came down to Spindle changing how the $1 in a bash script changed depending on whether it was passed in or out of spindle. This is related to #96, which was essentially the same issue, though this extends the fix into more corner cases. Specifically when argv[0] of the script does not match the argument passed to the first parameter of exec.

In addition, this adds a more comprehensive test case that tries to exec a set of scripts under a combinatorial set of circumstances (exec vs execve, argv[0] matches exec or not, path search vs relative vs absolute, and shell type).

@nchaimov
Copy link
Copy Markdown
Collaborator

Sorry, was trying out Copilot to see if it could find issues that I couldn't, and it posted without my permission! Not trying that again...

@mplegendre
Copy link
Copy Markdown
Member Author

Both those are valid finds, and good ones. The extra readlink is debugging code I accidently left in and the stat one is accurate. I'll fix those both.

It got some details wrong. The readlink not infinite recursion, just a performance hit, but still valid.

@nchaimov
Copy link
Copy Markdown
Collaborator

Regardless, it does look like Copilot found what I think may be a real problem, though: the wrapper for ldso_lxstat, to which a new fallback was added, falls back to plain stat. Is that intentional?

Comment thread src/client/client/exec_util.c Outdated
debug_printf2("Exec search request returned %s -> %s\n", newexec, *reloc_exec ? *reloc_exec : "NULL");
if (*reloc_exec) {
if (orig_file_abspath) {
*orig_file_abspath = strdup(newexec);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A question: the added code above in adjust_if_script uses spindle_strdup() (and replaces a strdup() with spindle_strdup(), but here strdup() is used. Under what circumstances should one be used vs the other?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'll move it to spindle_strdup() for consistency.

At one point I had a memory heap corruption checker integrated with the spindle, which required all of the client heap calls to go through wrappers (I had to do the interception manually since the library namespace issues kept spindle from working with regular tools).

./run_driver --serial ./interpreter_test
CHECK_RETCODE

./run_driver --serial ./exec_shell
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Unlike the other tests, this one doesn't print "PASSED." when it passes. For consistency, we might want to have it do that, so that it doesn't look like it crashed without printing.

Running: ./run_driver --serial ./interpreter_test
PASSED.
Running: ./run_driver --serial ./exec_shell
ALL TESTS PASSED

@mplegendre
Copy link
Copy Markdown
Member Author

I addressed the identified issues.
Thanks for the feedback on them.

@mplegendre mplegendre merged commit 1c5f1a4 into llnl:devel May 13, 2026
5 checks passed
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.

2 participants