Override shm-size with Docker and Podman Plug-ins#1025
Open
bluna301 wants to merge 3 commits into
Open
Conversation
Signed-off-by: will tepe <will.tepe@cchmc.org>
Signed-off-by: will tepe <will.tepe@cchmc.org>
|
Warning Review limit reached
More reviews will be available in 47 minutes and 35 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
WalkthroughA new Podman TaskManager plugin is introduced, mirroring the Docker plugin pattern but communicating with Podman's libpod HTTP API over a Unix domain socket. The Docker plugin is extended with ChangesPodman TaskManager Plugin + Docker shm_size
Sequence Diagram(s)sequenceDiagram
rect rgba(100, 149, 237, 0.5)
note over Caller,IContainerStatusMonitor: ExecuteTask
Caller->>PodmanPlugin: ExecuteTask(cancellationToken)
PodmanPlugin->>IStorageService: ListObjectsAsync / GetObjectAsync (inputs)
PodmanPlugin->>IDockerClient: CreateImageAsync (conditional pull)
PodmanPlugin->>IPodmanContainerCreator: CreateContainerAsync(podmanEndpoint, PodmanCreateContainerRequest)
IPodmanContainerCreator->>PodmanLibpodAPI: POST /v4.0.0/libpod/containers/create (Unix socket)
PodmanLibpodAPI-->>IPodmanContainerCreator: PodmanCreateContainerResponse
PodmanPlugin->>IDockerClient: StartContainerAsync(containerId)
PodmanPlugin->>IContainerStatusMonitor: Start(...) [background Task]
PodmanPlugin-->>Caller: ExecutionStatus.Accepted(containerId)
end
rect rgba(60, 179, 113, 0.5)
note over IContainerStatusMonitor,IMessageBrokerPublisher: Background monitoring
loop Poll until end state or timeout
IContainerStatusMonitor->>IDockerClient: InspectContainerAsync(containerId)
end
IContainerStatusMonitor->>IStorageService: PutObjectAsync(outputFiles)
IContainerStatusMonitor->>IMessageBrokerPublisher: PublishAsync(callbackTopic, TaskCallbackEvent)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: bluna301 <luna.bryanr@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
tests/UnitTests/TaskManager.Docker.Tests/DockerPluginTest.cs (1)
515-549: ⚡ Quick winAdd boundary tests for numeric-but-invalid values (
0, negative).Current tests only cover valid numeric and non-numeric strings. Please add cases asserting that
shm_size = "0"andshm_size = "-1"triggerInvalidShmSizeand do not setHostConfig.ShmSize.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/UnitTests/TaskManager.Docker.Tests/DockerPluginTest.cs` around lines 515 - 549, The existing test ExecuteTask_WhenShmSizeIsInvalid_ExpectLogWarningAndNoException in the DockerPluginTest class only covers non-numeric string inputs for shm_size. Add two new test methods following the same pattern to cover boundary cases: one test that sets shm_size to "0" and another that sets it to "-1", both asserting that they trigger an InvalidShmSize error and that HostConfig.ShmSize is not set on the container. Use similar setup and assertion patterns as the existing test but with these specific invalid numeric values.tests/UnitTests/TaskManager.Podman.Tests/PodmanPluginTest.cs (1)
535-617: ⚡ Quick winAdd regression coverage for
StartContainerAsync == falseand numeric invalidshm_sizevalues.Current tests cover non-numeric
shm_size, but not numeric invalid boundaries (0, negative) and not the container-start-false branch. These are important guards for execution-state correctness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/UnitTests/TaskManager.Podman.Tests/PodmanPluginTest.cs` around lines 535 - 617, The test suite currently covers valid shm_size and non-numeric invalid shm_size values, but is missing coverage for numeric invalid boundary cases (zero and negative values) and the failure path when StartContainerAsync returns false. Add two new test methods following the pattern of the existing ExecuteTask_WhenShmSizeIsInvalid_ExpectLogWarningAndNoException test: one that verifies behavior when shm_size is set to numeric invalid values like 0 or negative numbers (expecting appropriate error logging), and another that verifies the PodmanPlugin correctly handles the case when _podmanClient.Containers.StartContainerAsync returns false instead of true, ensuring the task result status and any error handling reflects this container startup failure.src/TaskManager/Plug-ins/Podman/Strings.cs (1)
32-32: ⚡ Quick winMake terminal-state constants immutable.
Line [32] exposes
DockerEndStatesas a mutableList<string>. Because completion logic depends on this set, runtime mutation can cause incorrect monitor behavior.♻️ Proposed change
- public static readonly List<string> DockerEndStates = new() { DockerStatusExited, DockerStatusDead }; + public static readonly IReadOnlyList<string> DockerEndStates = new[] { DockerStatusExited, DockerStatusDead };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/TaskManager/Plug-ins/Podman/Strings.cs` at line 32, The DockerEndStates field in the Strings class is exposed as a mutable List<string>, which allows runtime modifications that could cause incorrect monitor behavior since completion logic depends on this set. Change the type of DockerEndStates from List<string> to IReadOnlyList<string> to make it immutable while keeping the same initialization with DockerStatusExited and DockerStatusDead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/TaskManager/Plug-ins/Docker/DockerPlugin.cs`:
- Around line 370-379: The code at the `long.TryParse` check in the ShmSize
handling block accepts zero and negative values without validation. Add an
additional check after successful parsing to ensure the parsed shmsize value is
greater than 0 before assigning it to parameters.HostConfig.ShmSize. If the
parsed value is not positive (less than or equal to 0), log the error using
_logger.InvalidShmSize with the invalid value and skip the assignment, ensuring
only valid positive sizes are accepted.
In `@src/TaskManager/Plug-ins/Podman/ContainerStatusMonitor.cs`:
- Around line 186-193: The manual substring and trim operations on relativePart
at lines 186-189 and the string concatenation forming objectName at lines
191-192 can leak platform-specific path separators like backslashes into the
object key name, breaking object storage conventions. Replace the manual path
manipulation logic with C# Path APIs (such as Path.GetRelativePath or proper
path normalization) to ensure that both relativePart and objectName consistently
use forward slashes as separators, making the code platform-agnostic and
preventing downstream retrieval issues.
- Line 78: The ArgumentNullException.ThrowIfNull call on containerTimeout is
unnecessary because TimeSpan is a value type that cannot be null. More
importantly, this misses the actual validation issue: a zero or negative
TimeSpan will cause the retry loop at line 88 to skip entirely, resulting in
silent timeouts. Remove the ArgumentNullException.ThrowIfNull call for
containerTimeout and replace it with explicit range validation that ensures
containerTimeout is a positive TimeSpan value, throwing an appropriate exception
if the value is zero or negative.
In `@src/TaskManager/Plug-ins/Podman/IPodmanClientFactory.cs`:
- Around line 28-30: The CreateClient method must validate the Podman endpoint
scheme before creating the client to prevent unintended endpoint routing. In the
CreateClient method, add validation logic to check that the podmanEndpoint
parameter uses the unix:// scheme, which is required for Podman socket
connections. If the scheme is not unix://, throw an appropriate exception with a
descriptive error message before proceeding to call CreateClient on the
DockerClientConfiguration. This ensures that only valid unix domain socket
endpoints are accepted and prevents misconfigured endpoints from being used.
In `@src/TaskManager/Plug-ins/Podman/PodmanPlugin.cs`:
- Around line 482-486: The path construction on line 482 in the PodmanPlugin.cs
file is vulnerable to path traversal attacks because it uses simple string
replacement and trimming without validating that the resulting path stays within
the intended inputContainerDirRoot directory. An attacker could craft object
keys containing traversal sequences like "../" that would escape the container
directory and write files outside the intended task directory. To fix this,
after constructing the filePath using Path.Combine, normalize it with
Path.GetFullPath() and then validate that the normalized path starts with the
normalized inputContainerDirRoot path. If the validation fails, reject the file
path and log an error to prevent the directory creation and file write
operation.
- Around line 425-433: After successfully parsing the ShmSize value with
long.TryParse in the conditional block, add validation to reject non-positive
values (zero and negative numbers) before assigning to request.ShmSize. When the
parsed value is not greater than zero, log an appropriate error using _logger
(similar to the existing InvalidShmSize call for parse failures) and skip the
assignment. This ensures only valid positive shm_size values are accepted and
assigned to the request object.
- Around line 556-557: Replace the string concatenation approach used in the
chown Process.Start call with ProcessStartInfo that uses the ArgumentList
property to properly pass the user and path arguments separately, preventing
parsing issues if the user input contains whitespace or special characters.
Additionally, add a timeout parameter to the WaitForExit() call to prevent
indefinite blocking if the process hangs. Apply the same fix to the equivalent
chown execution in DockerPlugin.
---
Nitpick comments:
In `@src/TaskManager/Plug-ins/Podman/Strings.cs`:
- Line 32: The DockerEndStates field in the Strings class is exposed as a
mutable List<string>, which allows runtime modifications that could cause
incorrect monitor behavior since completion logic depends on this set. Change
the type of DockerEndStates from List<string> to IReadOnlyList<string> to make
it immutable while keeping the same initialization with DockerStatusExited and
DockerStatusDead.
In `@tests/UnitTests/TaskManager.Docker.Tests/DockerPluginTest.cs`:
- Around line 515-549: The existing test
ExecuteTask_WhenShmSizeIsInvalid_ExpectLogWarningAndNoException in the
DockerPluginTest class only covers non-numeric string inputs for shm_size. Add
two new test methods following the same pattern to cover boundary cases: one
test that sets shm_size to "0" and another that sets it to "-1", both asserting
that they trigger an InvalidShmSize error and that HostConfig.ShmSize is not set
on the container. Use similar setup and assertion patterns as the existing test
but with these specific invalid numeric values.
In `@tests/UnitTests/TaskManager.Podman.Tests/PodmanPluginTest.cs`:
- Around line 535-617: The test suite currently covers valid shm_size and
non-numeric invalid shm_size values, but is missing coverage for numeric invalid
boundary cases (zero and negative values) and the failure path when
StartContainerAsync returns false. Add two new test methods following the
pattern of the existing
ExecuteTask_WhenShmSizeIsInvalid_ExpectLogWarningAndNoException test: one that
verifies behavior when shm_size is set to numeric invalid values like 0 or
negative numbers (expecting appropriate error logging), and another that
verifies the PodmanPlugin correctly handles the case when
_podmanClient.Containers.StartContainerAsync returns false instead of true,
ensuring the task result status and any error handling reflects this container
startup failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 92e15090-66a3-4ff7-b1d9-1e471b76fec5
📒 Files selected for processing (26)
src/Common/Miscellaneous/ValidationConstants.cssrc/Monai.Deploy.WorkflowManager.slnsrc/TaskManager/Plug-ins/Docker/DockerPlugin.cssrc/TaskManager/Plug-ins/Docker/Keys.cssrc/TaskManager/Plug-ins/Docker/Logging/Log.cssrc/TaskManager/Plug-ins/Podman/AssemblyInfo.cssrc/TaskManager/Plug-ins/Podman/ContainerMonitorException.cssrc/TaskManager/Plug-ins/Podman/ContainerStatusMonitor.cssrc/TaskManager/Plug-ins/Podman/ContainerVolumeMount.cssrc/TaskManager/Plug-ins/Podman/IPodmanClientFactory.cssrc/TaskManager/Plug-ins/Podman/IPodmanContainerCreator.cssrc/TaskManager/Plug-ins/Podman/Keys.cssrc/TaskManager/Plug-ins/Podman/Logging/Log.cssrc/TaskManager/Plug-ins/Podman/Monai.Deploy.WorkflowManager.TaskManager.Podman.csprojsrc/TaskManager/Plug-ins/Podman/PodmanPlugin.cssrc/TaskManager/Plug-ins/Podman/SetPermissionException.cssrc/TaskManager/Plug-ins/Podman/Strings.cssrc/TaskManager/TaskManager/Extensions/TaskManagerExtensions.cssrc/TaskManager/TaskManager/Monai.Deploy.WorkflowManager.TaskManager.csprojsrc/TaskManager/TaskManager/PluginStrings.cssrc/TaskManager/TaskManager/packages.lock.jsontests/UnitTests/TaskManager.Docker.Tests/DockerPluginTest.cstests/UnitTests/TaskManager.Podman.Tests/ContainerStatusMonitorTest.cstests/UnitTests/TaskManager.Podman.Tests/Monai.Deploy.WorkflowManager.TaskManager.Podman.Tests.csprojtests/UnitTests/TaskManager.Podman.Tests/PodmanPluginTest.cstests/UnitTests/TaskManager.Podman.Tests/Usings.cs
Comment on lines
+370
to
+379
| if (Event.TaskPluginArguments.ContainsKey(Keys.ShmSize)) | ||
| { | ||
| if (long.TryParse(Event.TaskPluginArguments[Keys.ShmSize], out var shmsize)) | ||
| { | ||
| parameters.HostConfig.ShmSize = shmsize; | ||
| } | ||
| else | ||
| { | ||
| _logger.InvalidShmSize(Event.TaskPluginArguments[Keys.ShmSize]); | ||
| } |
There was a problem hiding this comment.
Enforce positive shm_size bounds, not just numeric parsing.
At Line 372, long.TryParse accepts 0 and negative values, but your own log contract says valid range starts at 1. This currently allows invalid values to be forwarded into HostConfig.ShmSize instead of being rejected/logged.
Suggested fix
- if (Event.TaskPluginArguments.ContainsKey(Keys.ShmSize))
- {
- if (long.TryParse(Event.TaskPluginArguments[Keys.ShmSize], out var shmsize))
- {
- parameters.HostConfig.ShmSize = shmsize;
- }
- else
- {
- _logger.InvalidShmSize(Event.TaskPluginArguments[Keys.ShmSize]);
- }
- }
+ if (Event.TaskPluginArguments.ContainsKey(Keys.ShmSize))
+ {
+ if (long.TryParse(Event.TaskPluginArguments[Keys.ShmSize], out var shmsize) && shmsize > 0)
+ {
+ parameters.HostConfig.ShmSize = shmsize;
+ }
+ else
+ {
+ _logger.InvalidShmSize(Event.TaskPluginArguments[Keys.ShmSize]);
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (Event.TaskPluginArguments.ContainsKey(Keys.ShmSize)) | |
| { | |
| if (long.TryParse(Event.TaskPluginArguments[Keys.ShmSize], out var shmsize)) | |
| { | |
| parameters.HostConfig.ShmSize = shmsize; | |
| } | |
| else | |
| { | |
| _logger.InvalidShmSize(Event.TaskPluginArguments[Keys.ShmSize]); | |
| } | |
| if (Event.TaskPluginArguments.ContainsKey(Keys.ShmSize)) | |
| { | |
| if (long.TryParse(Event.TaskPluginArguments[Keys.ShmSize], out var shmsize) && shmsize > 0) | |
| { | |
| parameters.HostConfig.ShmSize = shmsize; | |
| } | |
| else | |
| { | |
| _logger.InvalidShmSize(Event.TaskPluginArguments[Keys.ShmSize]); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/TaskManager/Plug-ins/Docker/DockerPlugin.cs` around lines 370 - 379, The
code at the `long.TryParse` check in the ShmSize handling block accepts zero and
negative values without validation. Add an additional check after successful
parsing to ensure the parsed shmsize value is greater than 0 before assigning it
to parameters.HostConfig.ShmSize. If the parsed value is not positive (less than
or equal to 0), log the error using _logger.InvalidShmSize with the invalid
value and skip the assignment, ensuring only valid positive sizes are accepted.
| CancellationToken cancellationToken = default) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(taskDispatchEvent, nameof(taskDispatchEvent)); | ||
| ArgumentNullException.ThrowIfNull(containerTimeout, nameof(containerTimeout)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify value-type null-guard misuse pattern and context
rg -n -C2 'ThrowIfNull\(containerTimeout' src/TaskManager/Plug-ins/Podman/ContainerStatusMonitor.cs
rg -n -C2 'TimeSpan containerTimeout' src/TaskManager/Plug-ins/Podman/ContainerStatusMonitor.csRepository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 840
🏁 Script executed:
# Inspect the method implementation to see how containerTimeout is used
sed -n '69,150p' src/TaskManager/Plug-ins/Podman/ContainerStatusMonitor.cs
# Check for similar null-guard patterns on other value types in the file
rg -n 'ThrowIfNull\(' src/TaskManager/Plug-ins/Podman/ContainerStatusMonitor.csRepository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 4224
Remove null-check and add timeout range validation.
containerTimeout is a TimeSpan (value type) and cannot be null, making line 78's ArgumentNullException.ThrowIfNull() a no-op. More critically, a zero or negative TimeSpan causes the retry loop (line 88) to skip entirely, silently timing out without error. Add explicit range validation instead:
- ArgumentNullException.ThrowIfNull(containerTimeout, nameof(containerTimeout));
+ if (containerTimeout <= TimeSpan.Zero)
+ {
+ throw new ArgumentOutOfRangeException(nameof(containerTimeout), "Container timeout must be greater than zero.");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ArgumentNullException.ThrowIfNull(containerTimeout, nameof(containerTimeout)); | |
| if (containerTimeout <= TimeSpan.Zero) | |
| { | |
| throw new ArgumentOutOfRangeException(nameof(containerTimeout), "Container timeout must be greater than zero."); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/TaskManager/Plug-ins/Podman/ContainerStatusMonitor.cs` at line 78, The
ArgumentNullException.ThrowIfNull call on containerTimeout is unnecessary
because TimeSpan is a value type that cannot be null. More importantly, this
misses the actual validation issue: a zero or negative TimeSpan will cause the
retry loop at line 88 to skip entirely, resulting in silent timeouts. Remove the
ArgumentNullException.ThrowIfNull call for containerTimeout and replace it with
explicit range validation that ensures containerTimeout is a positive TimeSpan
value, throwing an appropriate exception if the value is zero or negative.
Comment on lines
+186
to
+193
| var relativePart = file.StartsWith(artifactsPath, StringComparison.Ordinal) | ||
| ? file.Substring(artifactsPath.Length) | ||
| : file; | ||
| relativePart = relativePart.TrimStart('/'); | ||
| var objectName = string.IsNullOrEmpty(destination.RelativeRootPath) | ||
| ? relativePart | ||
| : destination.RelativeRootPath.TrimEnd('/') + "/" + relativePart; | ||
| _logger.UploadingFile(file, destination.Bucket, objectName); |
There was a problem hiding this comment.
Normalize relative artifact paths using path APIs before forming object keys.
At Lines 186-193, manual substring/trim can leak platform separators (\) into object names. That can break key conventions and downstream retrieval consistency.
Suggested fix
- var relativePart = file.StartsWith(artifactsPath, StringComparison.Ordinal)
- ? file.Substring(artifactsPath.Length)
- : file;
- relativePart = relativePart.TrimStart('/');
+ var relativePart = _fileSystem.Path.GetRelativePath(artifactsPath, file)
+ .Replace('\\', '/');
var objectName = string.IsNullOrEmpty(destination.RelativeRootPath)
? relativePart
: destination.RelativeRootPath.TrimEnd('/') + "/" + relativePart;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var relativePart = file.StartsWith(artifactsPath, StringComparison.Ordinal) | |
| ? file.Substring(artifactsPath.Length) | |
| : file; | |
| relativePart = relativePart.TrimStart('/'); | |
| var objectName = string.IsNullOrEmpty(destination.RelativeRootPath) | |
| ? relativePart | |
| : destination.RelativeRootPath.TrimEnd('/') + "/" + relativePart; | |
| _logger.UploadingFile(file, destination.Bucket, objectName); | |
| var relativePart = _fileSystem.Path.GetRelativePath(artifactsPath, file) | |
| .Replace('\\', '/'); | |
| var objectName = string.IsNullOrEmpty(destination.RelativeRootPath) | |
| ? relativePart | |
| : destination.RelativeRootPath.TrimEnd('/') + "/" + relativePart; | |
| _logger.UploadingFile(file, destination.Bucket, objectName); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/TaskManager/Plug-ins/Podman/ContainerStatusMonitor.cs` around lines 186 -
193, The manual substring and trim operations on relativePart at lines 186-189
and the string concatenation forming objectName at lines 191-192 can leak
platform-specific path separators like backslashes into the object key name,
breaking object storage conventions. Replace the manual path manipulation logic
with C# Path APIs (such as Path.GetRelativePath or proper path normalization) to
ensure that both relativePart and objectName consistently use forward slashes as
separators, making the code platform-agnostic and preventing downstream
retrieval issues.
Comment on lines
+28
to
+30
| public IDockerClient CreateClient(Uri podmanEndpoint) | ||
| { | ||
| return new DockerClientConfiguration(podmanEndpoint).CreateClient(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify endpoint flow and absence/presence of URI scheme validation.
rg -n -C3 --type=cs 'new Uri\(Event\.TaskPluginArguments\[Keys\.BaseUrl\]\)|CreateClient\(Uri podmanEndpoint\)|LocalPath|Scheme'Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 13811
🏁 Script executed:
# Search for PodmanClientFactory usage and any validation patterns
rg -n 'PodmanClientFactory|DockerClientConfiguration' --type=cs -B2 -A2 src/TaskManager/Plug-ins/Podman/Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 3039
🏁 Script executed:
# Look for unit tests related to PodmanClientFactory
fd -e '*Test*.cs' -e '*test*.cs' | xargs rg -l 'PodmanClientFactory|PodmanPlugin' 2>/dev/null | head -5Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 397
🏁 Script executed:
# Check if there's any validation in PodmanPlugin constructor or elsewhere
rg -n 'class PodmanPlugin|ArgumentException|ThrowIf' --type=cs src/TaskManager/Plug-ins/Podman/PodmanPlugin.cs -B1 -A5Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 1710
🏁 Script executed:
# Check the unit tests to see if they validate endpoint formats
cat tests/UnitTests/TaskManager.Podman.Tests/PodmanPluginTest.cs | head -100Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 5049
🏁 Script executed:
# Search for any endpoint validation in the entire Podman plugin folder
rg -n 'Scheme|unix|LocalPath|ArgumentException' --type=cs src/TaskManager/Plug-ins/Podman/ -B1 -A1Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 333
🏁 Script executed:
# Check what endpoints are used in tests to understand expected format
rg -n 'BaseUrl.*=|CreateClient.*Uri' tests/UnitTests/TaskManager.Podman.Tests/ --type=cs -B1 -A1Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 2132
🏁 Script executed:
# Search for how Event.TaskPluginArguments[Keys.BaseUrl] is set in tests
rg -n 'GenerateTaskDispatchEvent|BaseUrl' tests/UnitTests/TaskManager.Podman.Tests/PodmanPluginTest.cs -A3 | head -50Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 3216
🏁 Script executed:
# Find where ValidateEvent is defined and what it checks
rg -n 'private void ValidateEvent' --type=cs src/TaskManager/Plug-ins/Podman/PodmanPlugin.cs -A 30Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 1663
🏁 Script executed:
# Also check what happens with the BaseUrl validation in constructor
rg -n 'Keys.BaseUrl|ValidateEvent' --type=cs src/TaskManager/Plug-ins/Podman/PodmanPlugin.cs -B2 -A2Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 1517
Validate Podman endpoint scheme before creating the client.
The ValidateEvent() method validates that BaseUrl is a well-formed absolute URI (line 93-96), but does not enforce the unix:// scheme required for Podman socket connections. Since this endpoint originates from unchecked task arguments, add scheme validation to CreateClient() to prevent unintended endpoint routing and enforce that only unix domain sockets are accepted.
🛡️ Proposed fix
public IDockerClient CreateClient(Uri podmanEndpoint)
{
+ ArgumentNullException.ThrowIfNull(podmanEndpoint);
+ if (!string.Equals(podmanEndpoint.Scheme, "unix", StringComparison.OrdinalIgnoreCase) ||
+ string.IsNullOrWhiteSpace(podmanEndpoint.LocalPath))
+ {
+ throw new ArgumentException("Podman endpoint must be a unix domain socket URI.", nameof(podmanEndpoint));
+ }
return new DockerClientConfiguration(podmanEndpoint).CreateClient();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public IDockerClient CreateClient(Uri podmanEndpoint) | |
| { | |
| return new DockerClientConfiguration(podmanEndpoint).CreateClient(); | |
| public IDockerClient CreateClient(Uri podmanEndpoint) | |
| { | |
| ArgumentNullException.ThrowIfNull(podmanEndpoint); | |
| if (!string.Equals(podmanEndpoint.Scheme, "unix", StringComparison.OrdinalIgnoreCase) || | |
| string.IsNullOrWhiteSpace(podmanEndpoint.LocalPath)) | |
| { | |
| throw new ArgumentException("Podman endpoint must be a unix domain socket URI.", nameof(podmanEndpoint)); | |
| } | |
| return new DockerClientConfiguration(podmanEndpoint).CreateClient(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/TaskManager/Plug-ins/Podman/IPodmanClientFactory.cs` around lines 28 -
30, The CreateClient method must validate the Podman endpoint scheme before
creating the client to prevent unintended endpoint routing. In the CreateClient
method, add validation logic to check that the podmanEndpoint parameter uses the
unix:// scheme, which is required for Podman socket connections. If the scheme
is not unix://, throw an appropriate exception with a descriptive error message
before proceeding to call CreateClient on the DockerClientConfiguration. This
ensures that only valid unix domain socket endpoints are accepted and prevents
misconfigured endpoints from being used.
Comment on lines
+425
to
+433
| if (Event.TaskPluginArguments.ContainsKey(Keys.ShmSize)) | ||
| { | ||
| if (long.TryParse(Event.TaskPluginArguments[Keys.ShmSize], out var shmsize)) | ||
| { | ||
| request.ShmSize = shmsize; | ||
| } | ||
| else | ||
| { | ||
| _logger.InvalidShmSize(Event.TaskPluginArguments[Keys.ShmSize]); |
There was a problem hiding this comment.
Reject non-positive shm_size values before assigning.
Line 427 accepts any parsed long, including 0 and negatives, then sets request.ShmSize at Line 429. This contradicts the expected valid range and can produce invalid runtime requests.
Suggested fix
- if (long.TryParse(Event.TaskPluginArguments[Keys.ShmSize], out var shmsize))
+ if (long.TryParse(Event.TaskPluginArguments[Keys.ShmSize], NumberStyles.Integer, CultureInfo.InvariantCulture, out var shmsize) &&
+ shmsize > 0)
{
request.ShmSize = shmsize;
}
else
{
_logger.InvalidShmSize(Event.TaskPluginArguments[Keys.ShmSize]);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/TaskManager/Plug-ins/Podman/PodmanPlugin.cs` around lines 425 - 433,
After successfully parsing the ShmSize value with long.TryParse in the
conditional block, add validation to reject non-positive values (zero and
negative numbers) before assigning to request.ShmSize. When the parsed value is
not greater than zero, log an appropriate error using _logger (similar to the
existing InvalidShmSize call for parse failures) and skip the assignment. This
ensures only valid positive shm_size values are accepted and assigned to the
request object.
Comment on lines
+482
to
+486
| var filePath = Path.Combine(inputContainerDirRoot, obj.FilePath.Replace(input.RelativeRootPath, "").TrimStart('/')); | ||
|
|
||
| // Task Manager Container Path of the Directory for this Input File. | ||
| var fileDirectory = Path.GetDirectoryName(filePath); | ||
| Directory.CreateDirectory(fileDirectory!); |
There was a problem hiding this comment.
Block path traversal when writing downloaded input artifacts.
Line 482 builds local paths from object keys using string replacement. A key containing traversal segments can escape inputContainerDirRoot and write outside the intended task directory.
Suggested fix
- var filePath = Path.Combine(inputContainerDirRoot, obj.FilePath.Replace(input.RelativeRootPath, "").TrimStart('/'));
+ var relative = obj.FilePath.Replace(input.RelativeRootPath, string.Empty).TrimStart('/');
+ var root = Path.GetFullPath(inputContainerDirRoot + Path.DirectorySeparatorChar);
+ var filePath = Path.GetFullPath(Path.Combine(inputContainerDirRoot, relative));
+ if (!filePath.StartsWith(root, StringComparison.Ordinal))
+ {
+ throw new InvalidOperationException($"Invalid object path '{obj.FilePath}' for input '{input.Name}'.");
+ }
// Task Manager Container Path of the Directory for this Input File.
var fileDirectory = Path.GetDirectoryName(filePath);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/TaskManager/Plug-ins/Podman/PodmanPlugin.cs` around lines 482 - 486, The
path construction on line 482 in the PodmanPlugin.cs file is vulnerable to path
traversal attacks because it uses simple string replacement and trimming without
validating that the resulting path stays within the intended
inputContainerDirRoot directory. An attacker could craft object keys containing
traversal sequences like "../" that would escape the container directory and
write files outside the intended task directory. To fix this, after constructing
the filePath using Path.Combine, normalize it with Path.GetFullPath() and then
validate that the normalized path starts with the normalized
inputContainerDirRoot path. If the validation fails, reject the file path and
log an error to prevent the directory creation and file write operation.
Comment on lines
+556
to
+557
| var process = Process.Start("chown", $"-R {Event.TaskPluginArguments[Keys.User]} {path}"); | ||
| process.WaitForExit(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and examine lines around 556-557
find . -name "PodmanPlugin.cs" -type fRepository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 137
🏁 Script executed:
# Get the file location and examine the context
if [ -f "src/TaskManager/Plug-ins/Podman/PodmanPlugin.cs" ]; then
wc -l "src/TaskManager/Plug-ins/Podman/PodmanPlugin.cs"
echo "=== Lines 550-565 ===="
sed -n '550,565p' "src/TaskManager/Plug-ins/Podman/PodmanPlugin.cs" | cat -n
fiRepository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 1080
🏁 Script executed:
# Search for context on Event.TaskPluginArguments[Keys.User] and how it's populated
rg -n "TaskPluginArguments\[Keys.User\]" --context 5Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 5081
🏁 Script executed:
# Search for Keys.User definition and any validation
rg -n "Keys\.User" --context 3Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 4803
🏁 Script executed:
# Check for any input validation or sanitization related to user arguments
rg -n "chown" --context 10Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 4770
🏁 Script executed:
# Search for where Event.TaskPluginArguments is populated/assigned
rg -n "TaskPluginArguments\s*=" --context 3Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 32966
🏁 Script executed:
# Search for Keys class definition and any validation
rg -n "class Keys|enum Keys" --context 5Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 3329
🏁 Script executed:
# Look for any validation or sanitization of user input before it reaches SetPermission
rg -n "SetPermission" --context 5Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 12268
🏁 Script executed:
# Check if there's any documentation or validation around Keys.User in event/argument handling
rg -n "Keys\.User" -B5 -A2 --context 10 | head -100Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 8681
🏁 Script executed:
# Check the complete Keys.User definition in PodmanPlugin
cat src/TaskManager/Plug-ins/Podman/Keys.csRepository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 3541
🏁 Script executed:
# Verify how Process.Start behaves with string interpolation in C#/.NET
# Check if there are any existing uses of ProcessStartInfo with ArgumentList in the codebase
rg -n "ArgumentList|ProcessStartInfo" --context 3Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 1125
🏁 Script executed:
# Check if there are timeout mechanisms for other process calls in the codebase
rg -n "WaitForExit\(" --context 2Repository: Project-MONAI/monai-deploy-workflow-manager
Length of output: 1055
Use ProcessStartInfo with ArgumentList and timeout for chown execution.
Line 556 concatenates raw user input via string interpolation into the command arguments; if Keys.User contains whitespace or special characters, chown will misparse the arguments. Additionally, line 557's unbounded WaitForExit() can block indefinitely if the process hangs. The same issue exists in DockerPlugin.cs:505-506.
Suggested fix
- var process = Process.Start("chown", $"-R {Event.TaskPluginArguments[Keys.User]} {path}");
- process.WaitForExit();
+ var psi = new ProcessStartInfo("chown")
+ {
+ RedirectStandardError = true,
+ RedirectStandardOutput = true,
+ };
+ psi.ArgumentList.Add("-R");
+ psi.ArgumentList.Add(Event.TaskPluginArguments[Keys.User]);
+ psi.ArgumentList.Add(path);
+
+ using var process = Process.Start(psi) ?? throw new SetPermissionException("Failed to launch chown process.");
+ if (!process.WaitForExit(30_000))
+ {
+ process.Kill(entireProcessTree: true);
+ throw new SetPermissionException("chown command timed out.");
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/TaskManager/Plug-ins/Podman/PodmanPlugin.cs` around lines 556 - 557,
Replace the string concatenation approach used in the chown Process.Start call
with ProcessStartInfo that uses the ArgumentList property to properly pass the
user and path arguments separately, preventing parsing issues if the user input
contains whitespace or special characters. Additionally, add a timeout parameter
to the WaitForExit() call to prevent indefinite blocking if the process hangs.
Apply the same fix to the equivalent chown execution in DockerPlugin.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Allow the users to override the size of
/dev/shmwith the Docker and Podman plug-ins. Docker plug-in code copied from Victor's work in PR #978; code fixes adapted to Podman plug-in.Example usage (
shm_sizeinputs have unit of bytes):Status
Ready
Types of changes
Summary by CodeRabbit
Release Notes
/dev/shm) size support for Docker containers, enabling better control over container resource allocation.