lib/ukcpio: Fix symlinks not being extracted #992

andreittr · 2023-07-24T21:05:22Z

Description of changes

Previously the CPIO extraction code would only handle sections whose header was of type directory or file, skipping symlinks. This change adds code to handle extracting symlinks from CPIO archives, as well as a warning message for unknown CPIO sections.

Prerequisite checklist

Read the contribution guidelines regarding submitting new changes to the project;
Tested your changes against relevant architectures and platforms;
Ran the checkpatch.uk on your commit series before opening this PR;
Updated relevant documentation.

Base target

Architecture(s): N/A
Platform(s): N/A
Application(s): [N/A

Additional configuration

Test with an initrd root that contains symlinks.

Starnox

Not something that I used before but for sure can see the benefits. I tried an initRD config and it behaved as expected. As for the code, I don't have any issues with it, so from me, you have the green-light.

Reviewed-by: Eduard-Florin Mihailescu mihailescu.eduard@gmail.com

StefanJum

This looks good, few minor comments.
Besides those, when I try to run an application with a symlink in the initrd fs, I get the following error:

[    0.171998] Info: [libukcpio] <cpio.c @  298> Creating symlink /./file2
[    0.172907] Info: [libukcpio] <cpio.c @  316> /./file2: Target is file1
[    0.173839] dbg:  [libvfscore] <main.c @ 1532> (int) uk_syscall_r_symlink((const char*) 0x19beb0, (const char*) 0x1ac020)
[    0.175638] CRIT: [libvfscore] <vnode.c @  124> Assertion failure: m->owner != cur

Also please rebase this on top of staging so the ci/cd tests can run.

StefanJum · 2023-08-06T14:12:07Z

lib/ukcpio/cpio.c

+		target[header_filesize] = 0;
+
+		uk_pr_info("%s: Target is %s\n", path_from_root, target);
+		if (symlink(target, path_from_root)) {


Suggested change

if (symlink(target, path_from_root)) {

if (uk_syscall_r_symlink(target, path_from_root)) {

The rest of the file uses stdlib-like file access functions like open, write, chmod, etc., and I assume that's a design decision to not depend on syscall-shim. I kept the same convention for the symlink code.

The problem with libc-style syscalls is that you will exit the "kernel space" code and land into an external libc, that you can not control. There were some issues when this would lead to an infinite loop and other problems (see this pr), so we decided to use raw syscalls from whithin the Unikraft core, so the execution will not leave the core space.

The uk_syscall_* symbols are defined even if the syscall-shim is not enabled, so most likely the rest of the file was written before we ran into issues with the libc-style syscalls.

Right, I'm aware of the dangers of calling into libc from critical code but hadn't known about the raw syscall policy, nor that this file doesn't necessarily use libcalls on purpose; thanks for clarifying!

I'll use syscalls for symlink in this PR, and add fixing up the rest of the file (or module (or unikraft)) to my backlog to be handled in a future PR.

StefanJum · 2023-08-06T14:13:16Z

lib/ukcpio/cpio.c

+				(char *)(header) + sizeof(struct cpio_header)
+				+ header_namesize);
+
+		if ((uintptr_t)data_location + header_filesize > last) {


Suggested change

if ((uintptr_t)data_location + header_filesize > last) {

if (unlikely((uintptr_t)data_location + header_filesize > last)) {

I understand the point, however this being here only for the symlink case and not the others would suggest that symlinks are less likely to fail these checks compared to regular files or dirs (which isn't AFAICT accurate).
A cleaner way to integrate this change then would be to do it file- or lib-wide in its own changeset; thoughts?

Ok, makes sense, we can do this lib-wide.

StefanJum · 2023-08-06T14:13:38Z

lib/ukcpio/cpio.c

+		target[header_filesize] = 0;
+
+		uk_pr_info("%s: Target is %s\n", path_from_root, target);
+		if (symlink(target, path_from_root)) {


Suggested change

if (symlink(target, path_from_root)) {

if (unlikely(symlink(target, path_from_root))) {

andreittr · 2023-08-07T10:12:29Z

This looks good, few minor comments. Besides those, when I try to run an application with a symlink in the initrd fs, I get the following error:

@StefanJum You might need to apply this PR to fix symlink bugs in general: #952.

StefanJum · 2023-08-07T12:45:25Z

@StefanJum You might need to apply this PR to fix symlink bugs in general: #952.

Yes, that was it, it works now.

Previously the CPIO extraction code would only handle sections whose header was of type directory or file, skipping symlinks. This change adds code to handle extracting symlinks from CPIO archives, as well as a warning message for unknown CPIO sections. Signed-off-by: Andrei Tatar <andrei@unikraft.io>

andreittr · 2023-08-08T15:30:04Z

Update: rebased on staging; changed symlink to syscall
Deferring adding likely/unlikely to branches & changing over all other libcalls into syscalls to separate changesets.

StefanJum

All good now, thanks @andreittr 🚀
Reviewed-by: Stefan Jumarea stefanjumarea02@gmail.com

unikraft-bot · 2023-08-08T20:30:00Z

⚠️ Checkpatch passed with warnings

Beep boop! I ran Unikraft's checkpatch.pl support script on your pull request but it encountered warnings:

SHA	commit	checkpatch
`b01fa12`	lib/ukcpio: Fix symlinks not being extracted	⚠️

Truncated logs starting from first warning b01fa12:

WARNING:LINE_SPACING: Missing a blank line after declarations
#56: FILE: lib/ukcpio/cpio.c:313:
+		char target[header_filesize + 1];
+		memcpy(target, data_location, header_filesize);

total: 0 errors, 1 warnings, 57 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/tmp/build/a53d4c5b/patches/0001-lib-ukcpio-Fix-symlinks-not-being-extracted.patch has style problems, please review.

NOTE: Ignored message types: ASSIGN_IN_IF FILE_PATH_CHANGES NEW_TYPEDEFS OBSOLETE

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

View complete logs | Learn more about Unikraft's coding style and contribution guidelines.

razvand

Approved-by: Razvan Deaconescu razvand@unikraft.io

andreittr requested a review from a team as a code owner July 24, 2023 21:05

razvand requested review from Starnox, fabianpatras and calex257 and removed request for a team July 24, 2023 21:56

razvand self-assigned this Jul 24, 2023

razvand added area/lib Internal Unikraft Microlibrary kind/quick-fix Issue is a quick fix lang/c Issues or PRs to do with C/C++ priority/medium lib/ukcpio ukcpio: CPIO archive extraction labels Jul 24, 2023

razvand added this to the v0.14.0 (Prometheus) milestone Jul 24, 2023

razvand requested review from StefanJum and removed request for fabianpatras and calex257 August 5, 2023 05:36

Starnox approved these changes Aug 5, 2023

View reviewed changes

StefanJum requested changes Aug 6, 2023

View reviewed changes

andreittr force-pushed the ttr/cpio-symlink branch from a72d2fe to 7d910fc Compare August 7, 2023 10:35

andreittr force-pushed the ttr/cpio-symlink branch from 7d910fc to b01fa12 Compare August 8, 2023 15:28

andreittr requested a review from StefanJum August 8, 2023 15:30

StefanJum approved these changes Aug 8, 2023

View reviewed changes

razvand approved these changes Aug 9, 2023

View reviewed changes

unikraft-bot closed this in fbd70ca Aug 9, 2023

unikraft-bot added the ci/merged Merged by CI label Aug 9, 2023

andreittr deleted the ttr/cpio-symlink branch August 9, 2023 19:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib/ukcpio: Fix symlinks not being extracted #992

lib/ukcpio: Fix symlinks not being extracted #992

andreittr commented Jul 24, 2023

Starnox left a comment

StefanJum left a comment •

edited

StefanJum Aug 6, 2023

andreittr Aug 7, 2023

StefanJum Aug 7, 2023

andreittr Aug 7, 2023

StefanJum Aug 6, 2023

andreittr Aug 7, 2023

StefanJum Aug 7, 2023

StefanJum Aug 6, 2023

andreittr commented Aug 7, 2023

StefanJum commented Aug 7, 2023

andreittr commented Aug 8, 2023

StefanJum left a comment

unikraft-bot commented Aug 8, 2023

razvand left a comment

	if (symlink(target, path_from_root)) {
	if (uk_syscall_r_symlink(target, path_from_root)) {

	if ((uintptr_t)data_location + header_filesize > last) {
	if (unlikely((uintptr_t)data_location + header_filesize > last)) {

	if (symlink(target, path_from_root)) {
	if (unlikely(symlink(target, path_from_root))) {

lib/ukcpio: Fix symlinks not being extracted #992

lib/ukcpio: Fix symlinks not being extracted #992

Conversation

andreittr commented Jul 24, 2023

Description of changes

Prerequisite checklist

Base target

Additional configuration

Starnox left a comment

Choose a reason for hiding this comment

StefanJum left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreittr commented Aug 7, 2023

StefanJum commented Aug 7, 2023

andreittr commented Aug 8, 2023

StefanJum left a comment

Choose a reason for hiding this comment

unikraft-bot commented Aug 8, 2023

razvand left a comment

Choose a reason for hiding this comment

StefanJum left a comment •

edited