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

lib/vfscore: Fix behavior of writable open on directories #953

Closed
wants to merge 1 commit into from

Conversation

andreittr
Copy link
Contributor

Description of changes

Previously the open syscall, when called on a directory with write mode would either fail with EINVAL or, worse, finish successfully, whereas the expected behavior is to return EISDIR.
This change corrects this behavior, bringing open() 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): all
  • Platform(s): all
  • Application(s): all

Additional configuration

Minimal test code; all 4 cases fail with EISDIR on Linux, none should open the dir successfully.

int f;
FILE *fp;

f = open("/tmp", O_WRONLY);
if (f >= 0) {
	puts("open tmp OK");
	close(f);
} else {
	perror("open tmp");
}

f = open("/tmp", O_WRONLY|O_CREAT);
if (f >= 0) {
	puts("open tmp CREAT OK");
	close(f);
} else {
	perror("open tmp CREAT");
}

fp = fopen("/tmp", "wb");
if (fp) {
	puts("fopen tmp OK");
	fclose(fp);
} else {
	perror("fopen tmp");
}

fp = fopen("/tmp", "a");
if (fp) {
	puts("fopen tmp a OK");
	fclose(fp);
} else {
	perror("fopen tmp a");
}

@andreittr andreittr requested a review from a team as a code owner June 21, 2023 07:59
@unikraft-bot unikraft-bot added area/lib Internal Unikraft Microlibrary lang/c Issues or PRs to do with C/C++ lib/vfscore VFS Core Interface labels Jun 21, 2023
@unikraft-bot
Copy link
Member

Checkpatch passed

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

SHA commit checkpatch
ab99b0a lib/vfscore: Fix behavior of writable open on dirs

@razvand razvand assigned razvand and unassigned nderjung Jul 24, 2023
@razvand razvand requested review from dinhngtu and eduardvintila and removed request for a team July 24, 2023 20:29
@razvand razvand added this to the v0.14.0 (Prometheus) milestone Jul 24, 2023
@razvand razvand added the kind/quick-fix Issue is a quick fix label Jul 24, 2023
@andreittr andreittr changed the title lib/vfscore: Fix behavior of writable open on dirs lib/vfscore: Fix behavior of writable open on directories Jul 25, 2023
Copy link
Member

@dinhngtu dinhngtu left a comment

Choose a reason for hiding this comment

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

Hi, I've left a comment.

Copy link
Member

Choose a reason for hiding this comment

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

goto out_drele;
should be goto out_unlock;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh nice, good catch, thanks! Gonna fix asap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed fix.

Previously the open syscall, when called on a directory with write mode
would either fail with EINVAL or, worse, finish successfully, whereas
the expected behavior is to return EISDIR.
This change corrects this behavior, bringing open() in line with
expectations.

Signed-off-by: Andrei Tatar <andrei@unikraft.io>
@dinhngtu
Copy link
Member

dinhngtu commented Aug 2, 2023

That's all from me, thanks.

Reviewed-by: Tu Dinh Ngoc dinhngoc.tu@irit.fr

@dinhngtu dinhngtu self-requested a review August 7, 2023 08:52
Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

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

Approved-by: Razvan Deaconescu razvand@unikraft.io

@unikraft-bot unikraft-bot added the ci/merged Merged by CI label Aug 7, 2023
@andreittr andreittr deleted the ttr/open-eisdir branch August 7, 2023 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/lib Internal Unikraft Microlibrary ci/merged Merged by CI kind/quick-fix Issue is a quick fix lang/c Issues or PRs to do with C/C++ lib/vfscore VFS Core Interface
Projects
Status: Done!
Development

Successfully merging this pull request may close these issues.

None yet

5 participants