Skip to content

Multiple commits#3756

Merged
rhc54 merged 4 commits intoopenpmix:v5.0from
rhc54:cmr50/up
Dec 30, 2025
Merged

Multiple commits#3756
rhc54 merged 4 commits intoopenpmix:v5.0from
rhc54:cmr50/up

Conversation

@rhc54
Copy link
Contributor

@rhc54 rhc54 commented Dec 30, 2025

printf.c: fix off-by-one + underflow errors

Port of open-mpi/ompi#13592

Signed-off-by: Ralph Castain rhc@pmix.org
(cherry picked from commit 78af01e)

Workflow does not contain permissions

Per Copilot:

To fix the problem, explicitly declare a permissions block so that the GITHUB_TOKEN is limited to the minimal rights required. Since this workflow only checks out code and builds/tests it, it only needs read access to repository contents; no job appears to require write permissions or access to other scopes (issues, pull requests, packages, etc.).

The most straightforward fix that doesn’t alter existing behavior is to add a single, workflow-wide permissions block at the top level (same indentation as on: and jobs:) and set contents: read. This applies to all jobs that don’t override permissions, covering macos, ubuntu, and distcheck in one place. Concretely, in .github/workflows/builds.yaml, insert:

permissions:
contents: read

between the on: [pull_request] line and the jobs: line. No additional imports, methods, or other changes are required.

Signed-off-by: Ralph Castain rhc@pmix.org
(cherry picked from commit 77fc998)

Potentially overflowing call to snprintf (alert 11)

Per Copilot autofix:

In general, when using snprintf in a loop to build up a string, you must check each call’s return value before using it: if the return value is negative or greater than or equal to the remaining buffer size, you should stop writing and handle the error, rather than unconditionally adding the return value to your running length. This prevents len from being corrupted by a “would-have-written” size that is larger than the remaining space.

For this specific function, the safest fix with minimal functional change is:

Track the remaining space explicitly (remaining = max_ranks_len;) and always pass remaining as the size argument.
After each snprintf, if n < 0 or n >= remaining, treat it as failure: free the buffer, set *ranks = NULL, optionally log the error, and return early.
Only if n is valid, subtract it from remaining and increment len by n.
Because we are now checking after each call, the final if (len >= max_ranks_len - 1) becomes unnecessary, but we can keep a light sanity check if desired. To minimize changes, we can retain len and the final check, but make sure we never let len exceed max_ranks_len during the loop.

Concretely, in fill_seq_ranks_array in test/test_server.c, we will:

Introduce an int n; and a int remaining = max_ranks_len;.
Replace the two snprintf calls to:
    Call snprintf(*ranks + len, remaining, ...).
    Check if (n < 0 || n >= remaining) { free(*ranks); *ranks = NULL; TEST_ERROR(...); return; }.
    Update len += n; remaining -= n;.
Keep allocations and the if (0 >= nprocs) early-return logic unchanged.

No new methods or external libraries are required; we only reuse malloc, free, snprintf, and the existing TEST_ERROR macro.

Signed-off-by: Ralph Castain rhc@pmix.org
(cherry picked from commit b591fcb)

Potential double free and use after free (alerts 13,14)

Deallocating memory more than once can lead to a double-free vulnerability. This can be exploited to corrupt the allocator's internal data structures, which can lead to denial-of-service attacks by crashing the program, or security vulnerabilities, by allowing an attacker to overwrite arbitrary memory locations.

This rule finds accesses through a pointer of a memory location that has already been freed (i.e. through a dangling pointer). Such memory blocks have already been released to the dynamic memory manager, and modifying them can lead to anything from a segfault to memory corruption that would cause subsequent calls to the dynamic memory manager to behave erratically, to a possible security vulnerability.

Signed-off-by: Ralph Castain rhc@pmix.org
(cherry picked from commit db5b517)

Port of open-mpi/ompi#13592

Signed-off-by: Ralph Castain <rhc@pmix.org>
(cherry picked from commit 78af01e)
Per Copilot:

To fix the problem, explicitly declare a permissions block so that the GITHUB_TOKEN is limited to the minimal rights required. Since this workflow only checks out code and builds/tests it, it only needs read access to repository contents; no job appears to require write permissions or access to other scopes (issues, pull requests, packages, etc.).

The most straightforward fix that doesn’t alter existing behavior is to add a single, workflow-wide permissions block at the top level (same indentation as on: and jobs:) and set contents: read. This applies to all jobs that don’t override permissions, covering macos, ubuntu, and distcheck in one place. Concretely, in .github/workflows/builds.yaml, insert:

permissions:
  contents: read

between the on: [pull_request] line and the jobs: line. No additional imports, methods, or other changes are required.

Signed-off-by: Ralph Castain <rhc@pmix.org>
(cherry picked from commit 77fc998)
Per Copilot autofix:

In general, when using snprintf in a loop to build up a string, you must check each call’s return value before using it: if the return value is negative or greater than or equal to the remaining buffer size, you should stop writing and handle the error, rather than unconditionally adding the return value to your running length. This prevents len from being corrupted by a “would-have-written” size that is larger than the remaining space.

For this specific function, the safest fix with minimal functional change is:

    Track the remaining space explicitly (remaining = max_ranks_len;) and always pass remaining as the size argument.
    After each snprintf, if n < 0 or n >= remaining, treat it as failure: free the buffer, set *ranks = NULL, optionally log the error, and return early.
    Only if n is valid, subtract it from remaining and increment len by n.
    Because we are now checking after each call, the final if (len >= max_ranks_len - 1) becomes unnecessary, but we can keep a light sanity check if desired. To minimize changes, we can retain len and the final check, but make sure we never let len exceed max_ranks_len during the loop.

Concretely, in fill_seq_ranks_array in test/test_server.c, we will:

    Introduce an int n; and a int remaining = max_ranks_len;.
    Replace the two snprintf calls to:
        Call snprintf(*ranks + len, remaining, ...).
        Check if (n < 0 || n >= remaining) { free(*ranks); *ranks = NULL; TEST_ERROR(...); return; }.
        Update len += n; remaining -= n;.
    Keep allocations and the if (0 >= nprocs) early-return logic unchanged.

No new methods or external libraries are required; we only reuse malloc, free, snprintf, and the existing TEST_ERROR macro.

Signed-off-by: Ralph Castain <rhc@pmix.org>
(cherry picked from commit b591fcb)
Deallocating memory more than once can lead to a double-free vulnerability. This can be exploited to corrupt the allocator's internal data structures, which can lead to denial-of-service attacks by crashing the program, or security vulnerabilities, by allowing an attacker to overwrite arbitrary memory locations.

This rule finds accesses through a pointer of a memory location that has already been freed (i.e. through a dangling pointer). Such memory blocks have already been released to the dynamic memory manager, and modifying them can lead to anything from a segfault to memory corruption that would cause subsequent calls to the dynamic memory manager to behave erratically, to a possible security vulnerability.

Signed-off-by: Ralph Castain <rhc@pmix.org>
(cherry picked from commit db5b517)
@rhc54 rhc54 merged commit e87e285 into openpmix:v5.0 Dec 30, 2025
26 checks passed
@rhc54 rhc54 deleted the cmr50/up branch December 30, 2025 03:49
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.

1 participant