lib/vfscore: Fix create through broken symlink #998

andreittr · 2023-07-24T21:31:37Z

Description of changes

Previously a vfs syscall on a broken symlink would fail with ENOENT instead of attempting the syscall on the symlink target. This would make open(O_CREAT) to fail instead of creating the missing target file. This change fixes this behavior, bringing it in line with expectations.

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:

symlink("nonexistent", "broken-link");
int f = open("broken-link", O_WRONLY|O_CREAT, 0644);
if (f < 0) perror("bug in open()");

RaduNichita · 2023-07-30T12:29:12Z

lib/vfscore/lookup.c

+int
+namei(const char *path, struct dentry **dpp)


Suggested change

int

namei(const char *path, struct dentry **dpp)

int namei(const char *path, struct dentry **dpp)

You can leave it on the same line, as mentioned in the coding - conventions PR: unikraft/docs#248

Prefer the Linux style with the return type on the same line as the function name.
In case of long functions + first argument, you can break after the return type.

All of lookup.c uses this format, and I believe maintaining consistency is important.
If we however believe this style actively harms readability, we can address it for the entire file in a style change PR.

In vfs.h: sure, there it makes sense, and the rest of the file follows suit as well.

RaduNichita · 2023-07-30T12:30:12Z

lib/vfscore/vfs.h

+ *	- (0):  Completed successfully
+ *	- (<0): Negative value with error code
+ */
+int


RaduNichita · 2023-07-30T12:32:12Z

lib/vfscore/lookup.c

+	return error;
+}
+/*
+ * Convert a pathname into a pointer to a dentry


I think the commentary is not needed, since it's very similar in vfs.h

Indeed the comments are duplicates of the docstrings in vfs.h, however the file already has this style for exported functions and I don't want to break it without good reason.

mariasfiraiala

Thanks @andreittr. See my few comments below. Other than that I wasn't able to reproduce the issue with the example you provided, though the changes make sense.

mariasfiraiala · 2023-07-30T12:29:52Z

lib/vfscore/lookup.c

 */
 int
-namei(const char *path, struct dentry **dpp)
+namei_resolve(const char *path, struct dentry **dpp, char realpath[PATH_MAX])


Suggested change

namei_resolve(const char *path, struct dentry **dpp, char realpath[PATH_MAX])

namei_resolve(const char *path, struct dentry **dpp, char *realpath)

Let's have this without the specified size, just like it happens for the other argument.

mariasfiraiala · 2023-07-30T12:31:06Z

lib/vfscore/lookup.c

-	return 0;
+out:
+	if (realpath)
+		strcpy(realpath, fp);


Suggested change

strcpy(realpath, fp);

strlcpy(realpath, fp, PATH_MAX);

We can use strlcpy() in order to keep the consistency within the file and for better security.

andreittr · 2023-08-02T12:31:01Z

Thanks @andreittr. See my few comments below. Other than that I wasn't able to reproduce the issue with the example you provided, though the changes make sense.

What behavior are you getting when trying to reproduce? If the symlink call fails you might need to apply this PR: #952.

mariasfiraiala · 2023-08-02T13:53:20Z

If the symlink call fails you might need to apply this PR: #952.

I had already applied that PR since I also reviewed it.

What behavior are you getting when trying to reproduce?

The bug is not reproducible on 9pfs (meaning that the error message doesn't get printed), but with ramfs, the behaviour is indeed the one you mentioned, so all good.

@andreittr I'll approve the PR after you apply the suggestions, thank you for this!

Previously a vfs syscall on a broken symlink would fail with ENOENT instead of attempting the syscall on the symlink target. This would make open(O_CREAT) to fail instead of creating the missing target file. This change fixes this behavior, bringing it in line with expectations. Signed-off-by: Andrei Tatar <andrei@unikraft.io>

andreittr · 2023-08-02T14:35:04Z

Pushed changes addressing comments.

The bug is not reproducible on 9pfs (meaning that the error message doesn't get printed), but with ramfs, the behaviour is indeed the one you mentioned, so all good.

That's really odd that 9pfs isn't affected, as the code paths are common between filesystems 🤷 .

mariasfiraiala

All good on my side now, thanks a lot for the fixes @andreittr!

Reviewed-by: Maria Sfiraiala maria.sfiraiala@gmail.com

unikraft-bot · 2023-08-02T16:27:50Z

✅ Checkpatch passed

Beep boop! I ran Unikraft's checkpatch.pl support script on your pull request and it all looks good!

SHA	commit	checkpatch
`257ccba`	lib/vfscore: Fix create through broken symlink	✅

RaduNichita · 2023-08-02T21:33:57Z

All good also from my side. As @mariasfiraiala, I have only managed to reproduce the bug on ramfs, not 9pfs.

Reviewed-by: Radu Nichita radunichita99@gmail.com

razvand

Approved-by: Razvan Deaconescu razvand@unikraft.io

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

andreittr force-pushed the ttr/create-brsym branch from 81d6352 to 494b0eb Compare July 24, 2023 21:35

razvand requested review from RaduNichita and mariasfiraiala and removed request for a team July 24, 2023 21:49

razvand self-assigned this Jul 24, 2023

razvand added area/lib Internal Unikraft Microlibrary kind/quick-fix Issue is a quick fix lib/vfscore VFS Core Interface priority/medium lang/c Issues or PRs to do with C/C++ labels Jul 24, 2023

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

RaduNichita suggested changes Jul 30, 2023

View reviewed changes

mariasfiraiala suggested changes Jul 30, 2023

View reviewed changes

andreittr force-pushed the ttr/create-brsym branch from 494b0eb to 257ccba Compare August 2, 2023 14:31

mariasfiraiala approved these changes Aug 2, 2023

View reviewed changes

andreittr requested a review from RaduNichita August 2, 2023 15:09

RaduNichita approved these changes Aug 2, 2023

View reviewed changes

razvand approved these changes Aug 5, 2023

View reviewed changes

unikraft-bot closed this in 5f169bc Aug 7, 2023

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

andreittr deleted the ttr/create-brsym branch August 7, 2023 17:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib/vfscore: Fix create through broken symlink #998

lib/vfscore: Fix create through broken symlink #998

andreittr commented Jul 24, 2023

RaduNichita Jul 30, 2023

RaduNichita Jul 30, 2023

andreittr Aug 2, 2023 •

edited

Loading

RaduNichita Jul 30, 2023

RaduNichita Jul 30, 2023

andreittr Aug 2, 2023

mariasfiraiala left a comment

mariasfiraiala Jul 30, 2023

mariasfiraiala Jul 30, 2023

andreittr commented Aug 2, 2023

mariasfiraiala commented Aug 2, 2023

andreittr commented Aug 2, 2023

mariasfiraiala left a comment

unikraft-bot commented Aug 2, 2023

RaduNichita commented Aug 2, 2023

razvand left a comment

	int
	namei(const char path, struct dentry *dpp)
	int namei(const char path, struct dentry *dpp)

	namei_resolve(const char path, struct dentry *dpp, char realpath[PATH_MAX])
	namei_resolve(const char path, struct dentry dpp, char realpath)

lib/vfscore: Fix create through broken symlink #998

lib/vfscore: Fix create through broken symlink #998

Conversation

andreittr commented Jul 24, 2023

Description of changes

Prerequisite checklist

Base target

Additional configuration

RaduNichita Jul 30, 2023

Choose a reason for hiding this comment

RaduNichita Jul 30, 2023

Choose a reason for hiding this comment

andreittr Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

RaduNichita Jul 30, 2023

Choose a reason for hiding this comment

RaduNichita Jul 30, 2023

Choose a reason for hiding this comment

andreittr Aug 2, 2023

Choose a reason for hiding this comment

mariasfiraiala left a comment

Choose a reason for hiding this comment

mariasfiraiala Jul 30, 2023

Choose a reason for hiding this comment

mariasfiraiala Jul 30, 2023

Choose a reason for hiding this comment

andreittr commented Aug 2, 2023

mariasfiraiala commented Aug 2, 2023

andreittr commented Aug 2, 2023

mariasfiraiala left a comment

Choose a reason for hiding this comment

unikraft-bot commented Aug 2, 2023

RaduNichita commented Aug 2, 2023

razvand left a comment

Choose a reason for hiding this comment

andreittr Aug 2, 2023 •

edited

Loading