-
Notifications
You must be signed in to change notification settings - Fork 349
Add src-nofollow
& dest-nofollow
mount options
#1762
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
base: main
Are you sure you want to change the base?
Conversation
Reviewer's GuideThis PR adds two new bind mount options—src-nofollow and dest-nofollow—by extending the mount‐flags parser, propagating AT_SYMLINK_NOFOLLOW in get_bind_mount, implementing O_NOFOLLOW logic in do_mounts, updating is_bind_mount and CRIU support, and adding tests and documentation. Sequence Diagram: Bind Mount with
|
Change | Details | Files |
---|---|---|
Add src-nofollow and dest-nofollow keywords to the mount flags parser |
|
src/libcrun/mount_flags.c src/libcrun/mount_flags.h src/libcrun/mount_flags.perf |
Extend get_bind_mount and do_mounts to accept nofollow flags |
|
src/libcrun/linux.c src/libcrun/linux.h |
Update is_bind_mount and CRIU handling to detect and restrict nofollow |
|
src/libcrun/linux.c src/libcrun/criu.c |
Add automated tests for symlink nofollow behavior |
|
tests/test_mounts.py |
Document new src-nofollow and dest-nofollow options |
|
crun.1 crun.1.md |
Miscellaneous CI and feature updates |
|
.github/workflows/test.yaml tests/test_oci_features.py src/libcrun/cloned_binary.c |
Possibly linked issues
- #25947: The PR adds src-nofollow and dest-nofollow options to control symlink handling for bind mounts, addressing the issue.
Tips and commands
Interacting with Sourcery
- Trigger a new review: Comment
@sourcery-ai review
on the pull request. - Continue discussions: Reply directly to Sourcery's review comments.
- Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with@sourcery-ai issue
to create an issue from it. - Generate a pull request title: Write
@sourcery-ai
anywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai title
on the pull request to (re-)generate the title at any time. - Generate a pull request summary: Write
@sourcery-ai summary
anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment@sourcery-ai summary
on the pull request to
(re-)generate the summary at any time. - Generate reviewer's guide: Comment
@sourcery-ai guide
on the pull
request to (re-)generate the reviewer's guide at any time. - Resolve all Sourcery comments: Comment
@sourcery-ai resolve
on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore. - Dismiss all Sourcery reviews: Comment
@sourcery-ai dismiss
on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai review
to trigger a new review!
Customizing Your Experience
Access your dashboard to:
- Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others. - Change the review language.
- Add, remove or edit custom review instructions.
- Adjust other review settings.
Getting Help
- Contact our support team for questions or feedback.
- Visit our documentation for detailed guides and information.
- Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @giuseppe - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Variable 'target' is used uninitialized in the AT_SYMLINK_NOFOLLOW branch. (link)
General comments:
- Consider refactoring get_bind_mount’s growing list of boolean parameters (recursive, rdonly, nofollow) into a single flags bitmask or struct to improve readability and avoid further argument bloat.
- Rather than hand-editing mount_flags.c and hash values, hook the perf generator or mount_flags.perf into the build so that adding the new at_symlink_nofollow entry and its hash is automated.
- The new test creates a persistent symlink under the tests root without cleanup—consider using a temporary directory or teardown step to avoid leaving artifacts after the test runs.
Here's what I looked at during the review
- 🔴 General issues: 1 blocking issue, 2 other issues
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/libcrun/linux.c
Outdated
|
||
srcfd = ret; | ||
|
||
ret = safe_openat (rootfsfd, rootfs, target, O_PATH | O_NOFOLLOW | O_CLOEXEC, 0, err); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): Variable 'target' is used uninitialized in the AT_SYMLINK_NOFOLLOW branch.
'target' is passed to safe_openat without being initialized, which could cause undefined behavior. Please ensure 'target' is properly set before use.
src/libcrun/mount_flags.c
Outdated
@@ -218,6 +218,8 @@ static const struct propagation_flags_s wordlist[] = | |||
{"async", 1, MS_SYNCHRONOUS, 0}, | |||
#line 78 "src/libcrun/mount_flags.perf" | |||
{"rasync", 1, MS_SYNCHRONOUS, OPTION_RECURSIVE}, | |||
#line 93 "src/libcrun/mount_flags.perf" | |||
{"at_symlink_nofollow", 0, 0, OPTION_AT_SYMLINK_NOFOLLOW}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (bug_risk): No validation for mutually exclusive mount options.
Please add validation to prevent both 'copy_symlink' and 'at_symlink_nofollow' from being specified together, as this could cause conflicting behavior.
Suggested implementation:
/* ... existing option parsing logic ... */
/* After all options have been parsed, perform validation */
if ((options & OPTION_COPY_SYMLINK) && (options & OPTION_AT_SYMLINK_NOFOLLOW)) {
fprintf(stderr, "Error: 'copy_symlink' and 'at_symlink_nofollow' options are mutually exclusive.\n");
return -1;
}
- Ensure that
OPTION_COPY_SYMLINK
andOPTION_AT_SYMLINK_NOFOLLOW
are the correct flag names used in your codebase. Adjust them if your code uses different identifiers. - Place this validation after all mount options have been parsed and before any mount operation is performed.
- If your code uses a struct or different mechanism to track options, adapt the condition accordingly.
tests/test_mounts.py
Outdated
sys.stderr.write("got output %s\n" % out) | ||
if target_content in out: | ||
return 0 | ||
except Exception as e: | ||
sys.stderr.write("error %s\n" % e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: Consider removing diagnostic stderr
writes from the test.
These debug statements should be removed unless they're required for ongoing test analysis or CI logging.
tests/test_mounts.py
Outdated
if target_content in out: | ||
return 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests
)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
tests/test_mounts.py
Outdated
return 0 | ||
except Exception as e: | ||
sys.stderr.write("error %s\n" % e) | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Remove redundant pass statement (remove-redundant-pass
)
pass |
8315caf
to
64cdf3a
Compare
TMT tests failed. @containers/packit-build please check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this new option be documented in the runtime spec https://github.com/opencontainers/runtime-spec/blob/main/config.md#linux-mount-options?
There is already a nosymfollow documented there but I guess that was only intended for an actual fs mount and not bind mounts but maybe it could make sense to use the same name for both?
the nosymfollow is an actual option for the mount syscall and that applies to the new mount. When the mount is created with that option, the kernel doesn't follow any symlink there |
64cdf3a
to
7ef5644
Compare
we should first propose it, it doesn't go in automatically because it is not an option for the |
6dd83f6
to
3a8f3f8
Compare
@flouthoc @kolyshkin PTAL |
3a8f3f8
to
ffacd48
Compare
Yeah I meant to propose it. Personally I find |
yeah it is rather ugly, but that is the name of the @cyphar do you think this option would make sense for the OCI specs? Suggestions for a better name? :-) |
Well, a few thoughts:
|
6d58afd
to
df9e634
Compare
at_symlink_nofollow
bind mount option to mount symlinkssrc-nofollow
& dest-nofollow
mount options
Ephemeral COPR build failed. @containers/packit-build please check. |
TMT tests failed. @containers/packit-build please check. |
df9e634
to
987c057
Compare
thanks, that is a good idea. I've splitted the implementation into two different options:
Why would it be unsafe to do if we first open the destination with |
987c057
to
fcd403f
Compare
fcd403f
to
35282a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @giuseppe - I've reviewed your changes - here's some feedback:
- Rather than hand-editing mount_flags.c, update mount_flags.perf and regenerate the file with gperf to keep the generated hash tables in sync.
- The new get_bind_mount signature now has three bool flags—consider switching to a flags bitfield or struct to improve clarity and avoid signature bloat.
- Add a test that exercises the failure path when combining copy‐symlink with src‐nofollow or dest‐nofollow to ensure that mutually exclusive options are correctly rejected.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
if ((extra_flags & OPTION_COPY_SYMLINK) && (extra_flags & (OPTION_SRC_NOFOLLOW | OPTION_DEST_NOFOLLOW))) | ||
return crun_make_error (err, 0, "`copy-symlink` is mutually exclusive with `src-nofollow` and `dest-nofollow`"); | ||
|
||
/* Do not resolve the symlink only when src-nofollow and copy-symlink are used. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: Comment contradicts OR-based flag check
Update the comment to indicate that either flag triggers the condition, or change the logic to require both flags, so the comment and code are consistent.
for userns in [True, False]: | ||
for src_nofollow in [True, False]: | ||
conf = base_config() | ||
add_all_namespaces(conf, userns=userns) | ||
|
||
if userns: | ||
getMapping = lambda x : [ | ||
{ | ||
"containerID": 0, | ||
"hostID": x, | ||
"size": 1 | ||
} | ||
] | ||
conf['linux']['uidMappings'] = getMapping(os.geteuid()) | ||
conf['linux']['gidMappings'] = getMapping(os.getegid()) | ||
|
||
if src_nofollow: | ||
options = ["bind", "dest-nofollow", "src-nofollow"] | ||
conf['process']['args'] = ['/init', 'readlink', '/target'] | ||
expected = target_content | ||
else: | ||
options = ["bind", "dest-nofollow"] | ||
conf['process']['args'] = ['/init', 'cat', '/target'] | ||
expected = file_target_content | ||
|
||
mount_opt = {"destination": "/target", "type": "bind", "source": symlink, "options": options} | ||
conf['mounts'].append(mount_opt) | ||
|
||
try: | ||
out, _ = run_and_get_output(conf, hide_stderr=True,callback_prepare_rootfs=prepare_rootfs) | ||
sys.stderr.write("got output %s with configuration userns=%s, src-nofollow=%s\n" % (out, userns, src_nofollow)) | ||
if expected not in out: | ||
return -1 | ||
except Exception as e: | ||
sys.stderr.write("error %s\n" % e) | ||
return -1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests
)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
for src_nofollow in [True, False]: | ||
conf = base_config() | ||
add_all_namespaces(conf, userns=userns) | ||
|
||
if userns: | ||
getMapping = lambda x : [ | ||
{ | ||
"containerID": 0, | ||
"hostID": x, | ||
"size": 1 | ||
} | ||
] | ||
conf['linux']['uidMappings'] = getMapping(os.geteuid()) | ||
conf['linux']['gidMappings'] = getMapping(os.getegid()) | ||
|
||
if src_nofollow: | ||
options = ["bind", "dest-nofollow", "src-nofollow"] | ||
conf['process']['args'] = ['/init', 'readlink', '/target'] | ||
expected = target_content | ||
else: | ||
options = ["bind", "dest-nofollow"] | ||
conf['process']['args'] = ['/init', 'cat', '/target'] | ||
expected = file_target_content | ||
|
||
mount_opt = {"destination": "/target", "type": "bind", "source": symlink, "options": options} | ||
conf['mounts'].append(mount_opt) | ||
|
||
try: | ||
out, _ = run_and_get_output(conf, hide_stderr=True,callback_prepare_rootfs=prepare_rootfs) | ||
sys.stderr.write("got output %s with configuration userns=%s, src-nofollow=%s\n" % (out, userns, src_nofollow)) | ||
if expected not in out: | ||
return -1 | ||
except Exception as e: | ||
sys.stderr.write("error %s\n" % e) | ||
return -1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests
)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
if userns: | ||
getMapping = lambda x : [ | ||
{ | ||
"containerID": 0, | ||
"hostID": x, | ||
"size": 1 | ||
} | ||
] | ||
conf['linux']['uidMappings'] = getMapping(os.geteuid()) | ||
conf['linux']['gidMappings'] = getMapping(os.getegid()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests
)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
if src_nofollow: | ||
options = ["bind", "dest-nofollow", "src-nofollow"] | ||
conf['process']['args'] = ['/init', 'readlink', '/target'] | ||
expected = target_content | ||
else: | ||
options = ["bind", "dest-nofollow"] | ||
conf['process']['args'] = ['/init', 'cat', '/target'] | ||
expected = file_target_content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests
)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
for userns in [True, False]: | ||
for src_nofollow in [True, False]: | ||
conf = base_config() | ||
conf['process']['args'] = ['/init', 'cat', '/symlink'] | ||
add_all_namespaces(conf, userns=userns) | ||
|
||
if userns: | ||
getMapping = lambda x : [ | ||
{ | ||
"containerID": 0, | ||
"hostID": x, | ||
"size": 1 | ||
} | ||
] | ||
conf['linux']['uidMappings'] = getMapping(os.geteuid()) | ||
conf['linux']['gidMappings'] = getMapping(os.getegid()) | ||
|
||
if src_nofollow: | ||
options = ["bind", "dest-nofollow", "src-nofollow"] | ||
else: | ||
options = ["bind", "dest-nofollow"] | ||
mount_opt = {"destination": "/symlink", "type": "bind", "source": target, "options": options} | ||
conf['mounts'].append(mount_opt) | ||
|
||
try: | ||
out, _ = run_and_get_output(conf, hide_stderr=True,callback_prepare_rootfs=prepare_rootfs) | ||
sys.stderr.write("got output %s with configuration userns=%s, src-nofollow=%s\n" % (out, userns, src_nofollow)) | ||
if target_content not in out: | ||
return 1 | ||
except Exception as e: | ||
sys.stderr.write("error %s\n" % e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests
)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
for src_nofollow in [True, False]: | ||
conf = base_config() | ||
conf['process']['args'] = ['/init', 'cat', '/symlink'] | ||
add_all_namespaces(conf, userns=userns) | ||
|
||
if userns: | ||
getMapping = lambda x : [ | ||
{ | ||
"containerID": 0, | ||
"hostID": x, | ||
"size": 1 | ||
} | ||
] | ||
conf['linux']['uidMappings'] = getMapping(os.geteuid()) | ||
conf['linux']['gidMappings'] = getMapping(os.getegid()) | ||
|
||
if src_nofollow: | ||
options = ["bind", "dest-nofollow", "src-nofollow"] | ||
else: | ||
options = ["bind", "dest-nofollow"] | ||
mount_opt = {"destination": "/symlink", "type": "bind", "source": target, "options": options} | ||
conf['mounts'].append(mount_opt) | ||
|
||
try: | ||
out, _ = run_and_get_output(conf, hide_stderr=True,callback_prepare_rootfs=prepare_rootfs) | ||
sys.stderr.write("got output %s with configuration userns=%s, src-nofollow=%s\n" % (out, userns, src_nofollow)) | ||
if target_content not in out: | ||
return 1 | ||
except Exception as e: | ||
sys.stderr.write("error %s\n" % e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests
)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
if userns: | ||
getMapping = lambda x : [ | ||
{ | ||
"containerID": 0, | ||
"hostID": x, | ||
"size": 1 | ||
} | ||
] | ||
conf['linux']['uidMappings'] = getMapping(os.geteuid()) | ||
conf['linux']['gidMappings'] = getMapping(os.getegid()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests
)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
if src_nofollow: | ||
options = ["bind", "dest-nofollow", "src-nofollow"] | ||
else: | ||
options = ["bind", "dest-nofollow"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests
)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
if target_content not in out: | ||
return 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests
)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
@giuseppe I meant that this gives someone a very useful tool to overmount My plan in runc is to protect against this kind of thing with the safe proc API from libpathrs (which uses |
35282a5
to
2b12a37
Compare
thanks, that is a valid concern. I've added a new check now to prevent an overmount of a few file systems: procfs, sysfs and devpts (since this one is accessed within the container) |
2b12a37
to
093da2c
Compare
Signed-off-by: Giuseppe Scrivano <[email protected]>
just pass the value to the function call. Signed-off-by: Giuseppe Scrivano <[email protected]>
Signed-off-by: Giuseppe Scrivano <[email protected]>
This change enhances the `get_bind_mount` function by introducing a `nofollow` boolean parameter. When set to true, this parameter ensures that the `AT_SYMLINK_NOFOLLOW` flag is passed to the `syscall_open_tree` call. This allows the caller to specify that if the source of the bind mount is a symbolic link, the link itself should be mounted, rather than the target it points to. All existing call sites have been updated to pass `false` for the new `nofollow` parameter, thus preserving the previous behavior of following symlinks by default. This addition provides finer control for future use cases requiring specific handling of symbolic links during bind mount creation. Signed-off-by: Giuseppe Scrivano <[email protected]>
A local `source_mountfd` variable is introduced to store the file descriptor for the current mount entry. Signed-off-by: Giuseppe Scrivano <[email protected]>
This commit introduces the `dest-nofollow` and `src-nofollow` mount options. These new options allow for more precise control over how symbolic links are handled during mount-related operations, by enabling a "no-follow" behavior for symlinks at the specified path. The `mount_flags.c` file has been regenerated by gperf to include this new option in the lookup table. Signed-off-by: Giuseppe Scrivano <[email protected]>
now is_bind_mount returns whether the src-nofollow option was specified for the bind mount. Signed-off-by: Giuseppe Scrivano <[email protected]>
CRIU does not support the 'src-nofollow' option for bind mounts. Signed-off-by: Giuseppe Scrivano <[email protected]>
Introduce `src-nofollow` and `dest-nofollow` bind mount options for more precise control over symbolic link handling. The `src-nofollow` option enables mounting the source symbolic link itself, rather than its target. The `dest-nofollow` option ensures that if the destination path is a symbolic link, the mount operation replaces the symbolic link itself, instead of dereferencing it and mounting to its target. Closes: containers#1761 Signed-off-by: Giuseppe Scrivano <[email protected]>
093da2c
to
8bd2a31
Compare
Introduce
src-nofollow
anddest-nofollow
bind mount options for more precise control over symbolic link handling.The
src-nofollow
option enables mounting the source symbolic link itself, rather than its target.The
dest-nofollow
option ensures that if the destination path is a symbolic link, the mount operation replaces the symbolic link itself, instead of dereferencing it and mounting to its target.More details here: containers/podman#25947
Summary by Sourcery
Introduce a new
at_symlink_nofollow
bind mount option that mounts symlinks themselves instead of their targets by passing theAT_SYMLINK_NOFOLLOW
flag to theopen_tree(2)
syscall.New Features:
at_symlink_nofollow
mount option to bind mounts to prevent dereferencing of source symlinks.Enhancements:
get_bind_mount
and mounting logic to accept and handle the nofollow flag.at_symlink_nofollow
.Documentation:
at_symlink_nofollow
option in the man page and Markdown reference.Tests:
test_bind_mount_symlink_nofollow
to verify bind mounting of symlinks without following them.Summary by Sourcery
Introduce
src-nofollow
anddest-nofollow
bind mount options to control whether source or destination symlinks are mounted directly rather than dereferenced, and propagate these flags through the mounting logic.New Features:
src-nofollow
mount option to bind mounts to prevent dereferencing source symlinksdest-nofollow
mount option to bind mounts to replace destination symlinks instead of dereferencingEnhancements:
get_bind_mount
,do_mounts
,is_bind_mount
, and CRIU integration to accept and handle nofollow flagssrc-nofollow
anddest-nofollow
CI:
git diff
check to the CI workflowDocumentation:
src-nofollow
anddest-nofollow
options in the man page and Markdown referenceTests:
test_bind_mount_symlink_nofollow
andtest_bind_mount_file_nofollow
to verify nofollow behavior under various user namespacesChores:
src-nofollow
is used, since it isn’t supported