Skip to content
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.

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

use slices.Clone instead of assignment #24284

Merged
merged 1 commit into from
Oct 16, 2024

Conversation

zackattackz
Copy link
Contributor

Fixes #24267

This commit replaces a potentially unsafe slice-assignment with a call to slices.Clone.

This could prevent a bug where saveCommand and loadCommand could end up sharing an underlying array if parentFlags has a cap > it's len.

I did not write/change any tests for this PR, because it seems like a small enough change to be OK and because a test for this would be seem strange on the surface (passing in parentFlags with a cap > it's len to see if it causes issues). But if a test is needed I could amend one in at request.

Also, there could be other places in the code that could use this change, but I did not check. Some kind of linter for this would be ideal. gocritic did not seem to catch this when I checked.

Does this PR introduce a user-facing change?

None

Fixes containers#24267

This commit replaces a potentially unsafe slice-assignment with a call to `slices.Clone`.

This could prevent a bug where `saveCommand` and `loadCommand` could end up sharing an underlying array if `parentFlags` has a cap > it's len.

Signed-off-by: Zachary Hanham <z.hanham00@gmail.com>
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@rhatdan rhatdan added the No New Tests Allow PR to proceed without adding regression tests label Oct 15, 2024
@rhatdan
Copy link
Member

rhatdan commented Oct 15, 2024

Thanks @zackattackz
LGTM

@rhatdan
Copy link
Member

rhatdan commented Oct 15, 2024

/approve

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 15, 2024
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 16, 2024
Copy link
Contributor

openshift-ci bot commented Oct 16, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, rhatdan, zackattackz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit bd1abf0 into containers:main Oct 16, 2024
76 of 78 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. No New Tests Allow PR to proceed without adding regression tests release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Potential for buggy slice behavior
3 participants