Skip to content

Conversation

@LaxminarayanaV7416
Copy link

Proposed Changes

This PR is for BUG, where the problem is that from the factory, we are submitting the vine_worker command using the command line. When we include any special character, the POSIX command line cannot accept it, so we need to escape them and include a double quote, something like hello'world -> "hello'world" so then the POSIX will accept it. I am attaching with printf command while submitting the one without fix and with fix below for reference. NOTE that the printf is for dev and is not present in PR.

One without any fix
Screenshot 2025-08-27 at 7 01 56 PM

After the fix when we are able to submit the job
Screenshot 2025-08-27 at 7 02 54 PM

Merge Checklist

The following items must be completed before PRs can be merged.
Check these off to verify you have completed all steps.

  • make test Run local tests prior to pushing.
  • make format Format source code to comply with lint policies. Note that some lint errors can only be resolved manually (e.g., Python)
  • make lint Run lint on source code prior to pushing.
  • Manual Update: Update the manual to reflect user-visible changes.
  • Type Labels: Select a github label for the type: bugfix, enhancement, etc.
  • Product Labels: Select a github label for the product: TaskVine, Makeflow, etc.
  • PR RTM: Mark your PR as ready to merge.

Copy link
Member

@btovar btovar left a comment

Choose a reason for hiding this comment

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

I meant that you should try to use functions in dttools/src/stringtools.h if possible.

@btovar
Copy link
Member

btovar commented Sep 18, 2025

Is there progress in this pr?

@tphung3 tphung3 requested a review from Copilot September 22, 2025 18:19
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug in the vine_factory where special characters in command line arguments were not being properly escaped for POSIX shell submission, causing command execution failures. The fix introduces proper shell escaping by wrapping arguments in double quotes and escaping special characters.

Key changes:

  • Added a new string_escape_shell_vine_factory() function to properly escape shell special characters
  • Applied shell escaping to submission_regex, manager_host, worker_command, and scratch_dir parameters in command construction
  • Removed single quotes around arguments in cp commands since the escaping function now handles proper quoting

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +225 to +226
// Worst case: every char becomes 2 chars + quotes
char *output = malloc(len * 2 + 3);
Copy link

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

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

The memory allocation calculation len * 2 + 3 may be insufficient for strings containing many special characters. Consider using a more conservative calculation or dynamic reallocation to prevent buffer overflows.

Suggested change
// Worst case: every char becomes 2 chars + quotes
char *output = malloc(len * 2 + 3);
// First pass: calculate required output size
size_t out_len = 2; // for opening and closing quotes
for (const char *src = input; *src; src++) {
switch (*src) {
case '"':
case '\\':
case '$':
case '`':
case '!':
case '\'':
out_len += 2; // escaped char: backslash + char
break;
default:
out_len += 1;
break;
}
}
out_len += 1; // for null terminator
char *output = malloc(out_len);

Copilot uses AI. Check for mistakes.
"./%s --parent-death -M %s -t %d -C '%s' %s %s %s %s %s %s %s %s",
worker_command,
submission_regex,
string_escape_shell_vine_factory(submission_regex),
Copy link

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

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

Memory leak: The strings returned by string_escape_shell_vine_factory() are dynamically allocated but never freed. Store the returned pointers and call free() after string_format() to prevent memory leaks.

Copilot uses AI. Check for mistakes.
"./%s --parent-death %s %d -t %d -C '%s' %s %s %s %s %s %s %s",
worker_command,
manager_host,
string_escape_shell_vine_factory(manager_host),
Copy link

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

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

Memory leak: The strings returned by string_escape_shell_vine_factory() are dynamically allocated but never freed. Store the returned pointers and call free() after string_format() to prevent memory leaks.

Copilot uses AI. Check for mistakes.
if(worker_command != NULL){
cmd = string_format("cp '%s' '%s'",worker_command,scratch_dir);
cmd = string_format("cp %s %s",
string_escape_shell_vine_factory(worker_command),
Copy link

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

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

Memory leak: The strings returned by string_escape_shell_vine_factory() are dynamically allocated but never freed. Store the returned pointers and call free() after string_format() to prevent memory leaks.

Copilot uses AI. Check for mistakes.
Comment on lines +1737 to +1738
string_escape_shell_vine_factory(tmp),
string_escape_shell_vine_factory(scratch_dir));
Copy link

Copilot AI Sep 22, 2025

Choose a reason for hiding this comment

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

Memory leak: The strings returned by string_escape_shell_vine_factory() are dynamically allocated but never freed. Store the returned pointers and call free() after string_format() to prevent memory leaks.

Copilot uses AI. Check for mistakes.
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