Do not allow truncating files which are not opened for writing.

[marmistrz]

This came up in #557. I don't know if the Windows implementation is affected.

cc @peterhuene@kubkon@sunfishcode

[peterhuene]

Currently the truncation_rights test is expecting to be able to open a file for truncation without passing in a right that would cause the file to be opened for write, which is why it is failing.

Also why I made the change that I did in #557.

After more discussion, I think the WASI spec is correct and the test case needs to be fixed to assert the __WASI_RIGHT_PATH_FILESTAT_SET_SIZE right for passing in O_TRUNC.

[sunfishcode]

That looks right to me. Do we have a test for this in crates/wasi-misc-tests?

[marmistrz]

No, currently we only check if we can truncate the file without __WASI_RIGHT_PATH_FILESTAT_SET_SIZE, which is correct, as long as we still have __WASI_RIGHT_FD_WRITE. Moreover, it's possible that even if we had a test for it, Linux would behave in a sane way and disallow the truncation.

The way I see it, we should do four tests.

1. O_TRUNC works with the default flags
2. O_TRUNC works with __WASI_RIGHT_PATH_FILESTAT_SET_SIZE dropped
3. O_TRUNC works with __WASI_RIGHT_FD_WRITE dropped
4. O_TRUNC doesn't with both dropped and returns __WASI_ENOTCAPABLE.

On the other hand, I'm currently confused about the current implementation. The way I understand what it does.

We pass 0 to path_open, which means no rights. We're working in this no rights setup and we add rights in path_open_rights by hand depending not on the inherited rights, but on the requested oflags (sic!), not even checking if we can inherit those rights. read/write are calculated based on the input arguments (zeros), though. At the very end, we fix the rights up by doing a binary AND with the inheriting rights.

This sounds completely wrong.

Anyway, I don't really get what initial rights mean, as used in the WASI spec.

[kubkon]

No, currently we only check if we can truncate the file without __WASI_RIGHT_PATH_FILESTAT_SET_SIZE, which is correct, as long as we still have __WASI_RIGHT_FD_WRITE. Moreover, it's possible that even if we had a test for it, Linux would behave in a sane way and disallow the truncation.

The way I see it, we should do four tests.

1. `O_TRUNC` works with the default flags
2. `O_TRUNC` works with `__WASI_RIGHT_PATH_FILESTAT_SET_SIZE` dropped
3. `O_TRUNC` works with `__WASI_RIGHT_FD_WRITE` dropped
4. `O_TRUNC` doesn't with both dropped and returns `__WASI_ENOTCAPABLE`.

Would you mind modifying the test to include this then?

On the other hand, I'm currently confused about the current implementation. The way I understand what it does.

We pass 0 to path_open, which means no rights. We're working in this no rights setup and we add rights in path_open_rights by hand depending not on the inherited rights, but on the requested oflags (sic!), not even checking if we can inherit those rights. read/write are calculated based on the input arguments (zeros), though. At the very end, we fix the rights up by doing a binary AND with the inheriting rights.

This sounds completely wrong.

By "we pass 0 to path_open", do you mean the rights_base or rights_inheriting, or both? In any case, both sets of rights target the created file descriptor. But the way it is currently verified that the created file descriptor will indeed possess those rights is through the dirfd we open the path against. So rights_base and rights_inheriting passed to path_open are used to verify that the dirfd we currently operate on has the correct inheriting rights set on itself (which if satisfied, eliminates the need to make any additional rights checks/modifications on the opened file descriptor since it automatically satisfies all of those by virtue of being a derivative of the dirfd). Hence, if we set rights_base to __WASI_RIGHT_FD_READ, we then ensure that the derived fd will indeed possess those rights, etc.

BTW, did you guys realise that, on Unix, you can path_open with both rights set to 0 and you'd still implicitly inherit __WASI_RIGHT_FD_READ? This is by virtue of open always requiring any of the O_RDONLY, O_WRONLY, or O_RDWR :-D But then, if you specify __WASI_RIGHT_FD_WRITE, we don't implicitly inherit the read right. So all in all, I guess we can treat __WASI_RIGHT_FD_READ as a flag we can use to discriminate between write only or read-write.

Anyway, I don't really get what initial rights mean, as used in the WASI spec.

AFAIK initial in this context means that they are the maximum rights the fd can have, but the implementation can strip some of them away provided they don't conflict with the minimum required by the type of the fd.

Anyhow, that's my understanding. I hope I didn't confuse anyone too much. @sunfishcode, as the original creator of wasi-c which the path_open is essentially taken from, would you mind chipping in here? :-)

[kubkon]

LGTM!

[peterhuene]

By "we pass 0 to path_open", do you mean the rights_base or rights_inheriting, or both?

The truncation_rights test is passing 0 for both, and expecting that inheriting __WASI_RIGHT_PATH_FILESTAT_SET_SIZE and __WASI_RIGHT_PATH_OPEN from the directory fd is sufficient to open the file __WASI_O_TRUNC.

However, on all platforms, write access to the file being created is required to properly truncate a file.

We only check the flags passed to path_open to see if the file should be writable. Since the test passes 0, the file doesn't open for write and the open fails.

So is the test wrong and should pass in a right to cause the file to be open writable or should we be deducing that it should open writable if we're inheriting __WASI_RIGHT_PATH_FILESTAT_SET_SIZE?

[marmistrz]

No, currently we only check if we can truncate the file without __WASI_RIGHT_PATH_FILESTAT_SET_SIZE, which is correct, as long as we still have __WASI_RIGHT_FD_WRITE. Moreover, it's possible that even if we had a test for it, Linux would behave in a sane way and disallow the truncation.
The way I see it, we should do four tests.

1. `O_TRUNC` works with the default flags
2. `O_TRUNC` works with `__WASI_RIGHT_PATH_FILESTAT_SET_SIZE` dropped
3. `O_TRUNC` works with `__WASI_RIGHT_FD_WRITE` dropped
4. `O_TRUNC` doesn't with both dropped and returns `__WASI_ENOTCAPABLE`.

Would you mind modifying the test to include this then?

Sure, I'll modify the test. One problem I can see is that currently fdstat_set_rights only allows dropping rights (which makes sense) and WASI doesn't support dup. Probably the only way to do this is to create another file descriptor with path_open-ing the path ..

On the other hand, I'm currently confused about the current implementation. The way I understand what it does.
We pass 0 to path_open, which means no rights. We're working in this no rights setup and we add rights in path_open_rights by hand depending not on the inherited rights, but on the requested oflags (sic!), not even checking if we can inherit those rights. read/write are calculated based on the input arguments (zeros), though. At the very end, we fix the rights up by doing a binary AND with the inheriting rights.
This sounds completely wrong.

By "we pass 0 to path_open", do you mean the rights_base or rights_inheriting, or both? In any case, both sets of rights target the created file descriptor. But the way it is currently verified that the created file descriptor will indeed possess those rights is through the dirfd we open the path against. So rights_base and rights_inheriting passed to path_open are used to verify that the dirfd we currently operate on has the correct inheriting rights set on itself (which if satisfied, eliminates the need to make any additional rights checks/modifications on the opened file descriptor since it automatically satisfies all of those by virtue of being a derivative of the dirfd). Hence, if we set rights_base to __WASI_RIGHT_FD_READ, we then ensure that the derived fd will indeed possess those rights, etc.

Well, I meant both rights_base and rights_inheriting. In other words, if the parent doesn't possess the rights mentioned as rights_base or rights_inheriting, path_open should return with an error, right?

I still claim that we're passing needed_base to path_get and these needed rights are used to validate if the required rights are present. (the flow is: path_get -> as_descriptor -> validate_rights) Similarly, both read and write should evaluate to 0, because fs_rights_base == 0. Both behaviors sounds wrong.

BTW, did you guys realise that, on Unix, you can path_open with both rights set to 0 and you'd still implicitly inherit __WASI_RIGHT_FD_READ? This is by virtue of open always requiring any of the O_RDONLY, O_WRONLY, or O_RDWR :-D But then, if you specify __WASI_RIGHT_FD_WRITE, we don't implicitly inherit the read right. So all in all, I guess we can treat __WASI_RIGHT_FD_READ as a flag we can use to discriminate between write only or read-write.

Anyway, I don't really get what initial rights mean, as used in the WASI spec.

AFAIK initial in this context means that they are the maximum rights the fd can have, but the implementation can strip some of them away provided they don't conflict with the minimum required by the type of the fd.

Then with 0 we should have absolutely no rights and we shouldn't be able to read any files at all. I skimmed through the misc tests and we don't seem to have a test for it. Anyway, this is completely opposite from what you suggested about ensuring stuff.

[kubkon]

No, currently we only check if we can truncate the file without __WASI_RIGHT_PATH_FILESTAT_SET_SIZE, which is correct, as long as we still have __WASI_RIGHT_FD_WRITE. Moreover, it's possible that even if we had a test for it, Linux would behave in a sane way and disallow the truncation.
The way I see it, we should do four tests.

1. `O_TRUNC` works with the default flags
2. `O_TRUNC` works with `__WASI_RIGHT_PATH_FILESTAT_SET_SIZE` dropped
3. `O_TRUNC` works with `__WASI_RIGHT_FD_WRITE` dropped
4. `O_TRUNC` doesn't with both dropped and returns `__WASI_ENOTCAPABLE`.

Would you mind modifying the test to include this then?

Sure, I'll modify the test. One problem I can see is that currently fdstat_set_rights only allows dropping rights (which makes sense) and WASI doesn't support dup. Probably the only way to do this is to create another file descriptor with path_open-ing the path ..

Yeah, AFAIK in WASI we can only every drop rights, never add more. So I guess you could do two opens, dropping __WASI_RIGHT_PATH_FILESTAT_SET_SIZE in one, and then __WASI_RIGHT_FD_WRITE in the other, and verify that we can still truncate, right?

On the other hand, I'm currently confused about the current implementation. The way I understand what it does.
We pass 0 to path_open, which means no rights. We're working in this no rights setup and we add rights in path_open_rights by hand depending not on the inherited rights, but on the requested oflags (sic!), not even checking if we can inherit those rights. read/write are calculated based on the input arguments (zeros), though. At the very end, we fix the rights up by doing a binary AND with the inheriting rights.
This sounds completely wrong.

By "we pass 0 to path_open", do you mean the rights_base or rights_inheriting, or both? In any case, both sets of rights target the created file descriptor. But the way it is currently verified that the created file descriptor will indeed possess those rights is through the dirfd we open the path against. So rights_base and rights_inheriting passed to path_open are used to verify that the dirfd we currently operate on has the correct inheriting rights set on itself (which if satisfied, eliminates the need to make any additional rights checks/modifications on the opened file descriptor since it automatically satisfies all of those by virtue of being a derivative of the dirfd). Hence, if we set rights_base to __WASI_RIGHT_FD_READ, we then ensure that the derived fd will indeed possess those rights, etc.

Well, I meant both rights_base and rights_inheriting. In other words, if the parent doesn't possess the rights mentioned as rights_base or rights_inheriting, path_open should return with an error, right?

That's correct. If the parent fd doesn't possess the combination of both, then we should throw ENOTCAPABLE.

I still claim that we're passing needed_base to path_get and these needed rights are used to validate if the required rights are present. (the flow is: path_get -> as_descriptor -> validate_rights) Similarly, both read and write should evaluate to 0, because fs_rights_base == 0. Both behaviors sounds wrong.

Hmm, but the required rights are part of the needed_base, and path_get is responsible for returning a new host descriptor, so if the dirfd fails the validation, it will correctly fail in path_get with a ENOTCAPABLE.

BTW, did you guys realise that, on Unix, you can path_open with both rights set to 0 and you'd still implicitly inherit __WASI_RIGHT_FD_READ? This is by virtue of open always requiring any of the O_RDONLY, O_WRONLY, or O_RDWR :-D But then, if you specify __WASI_RIGHT_FD_WRITE, we don't implicitly inherit the read right. So all in all, I guess we can treat __WASI_RIGHT_FD_READ as a flag we can use to discriminate between write only or read-write.

Anyway, I don't really get what initial rights mean, as used in the WASI spec.

AFAIK initial in this context means that they are the maximum rights the fd can have, but the implementation can strip some of them away provided they don't conflict with the minimum required by the type of the fd.

Then with 0 we should have absolutely no rights and we shouldn't be able to read any files at all. I skimmed through the misc tests and we don't seem to have a test for it. Anyway, this is completely opposite from what you suggested about ensuring stuff.

Actually, the 0 rights quirk comes directly from the POSIX world. On Linux you're also never able to open for neither reading nor writing; you have specify at least one. I'm wary that enforcing absolutely no rights would require a middle-layer that would track those rights, i.e., our implementation would have to do this heavy lifting rather then relying on the host to this for us. This might be the way to go but would probably require agreeing on a strategy on how to handle it first.

Well, I believe we still ensure we possess the right rights by checking that the dirfd possesses the right inheriting rights.

[marmistrz]

@kubkon, you were right about the fact that we're correctly validating the permissions in path_get. Sorry for that.

The semantics of the path_open rights are still confusing to me. In light of what you said I view them rather as assertions, which guarantee that our descriptor will indeed possess all the rights that we need. On the other hand, we don't seem to inherit all the rights from the parent descriptor anywhere, we're rather getting the explicitly requested ones and the basic rights dependent on the file type.

So the question: for any inheriting right of the parent descriptor, when should the child descriptor also have this right? Always or only when requested explicitly?

I attempted to sanitize path_open so that it's always called with at least one of read or write set to true: https://github.com/marmistrz/wasmtime/tree/open.
The problem is that path_open doesn't expect the caller to provide the open mode beforehand, so the best we can do is to get all the modes possible. Unfortunately, that's not fine either, since one can't open a directory for writing, and we don't know whether it's a directory until we call openat, so checking that would require another syscall. This sounds completely wrong.

... we really do need more precise docs about the semantics of path_open parameters...

[kubkon]

I attempted to sanitize path_open so that it's always called with at least one of read or write set to true: https://github.com/marmistrz/wasmtime/tree/open.
The problem is that path_open doesn't expect the caller to provide the open mode beforehand, so the best we can do is to get all the modes possible. Unfortunately, that's not fine either, since one can't open a directory for writing, and we don't know whether it's a directory until we call openat, so checking that would require another syscall. This sounds completely wrong.

I wouldn't say completely wrong, rather, unfortunate. As we discussed above, Linux/Unix are not much different in that they also require at least one of read,write,both but never none (I guess it wouldn't make sense to open without at least one). Anyhow, I have a different proposition. Perhaps we shouldn't be implicit about the read right, but explicit, and if none of the above is specified, simply throw an EINVAL? This would be in line with the way Rust does it for instance. Here's the relevant link. @sunfishcode@peterhuene thoughts?

... we really do need more precise docs about the semantics of path_open parameters...

Agreed, better docs with some examples/guides would go a long way. But we can probably work that out as we go along.

[marmistrz]

@kubkon, the problem is that the public API has no absolutely no way of indicating read/write intent. Probably we should add more flags to __wasi_oflags_t, but this will require a spec change. read/write is only in our pub(crate) API.

[marmistrz]

I think I know what the initial from the WASI spec could mean: that these are the rights that the fd will possess until the rights are set using __wasi_fd_fdstat_set_rights(). But I'm not sure, and I'd like to have this confirmed.

[peterhuene]

@marmistrz@kubkon@sunfishcode

So what do we think the correct course of action is with this PR?

The failing truncation_rights test is also blocking #557 as the test simply won't pass as currently implementation as truncation of a file will require write access on all platforms.

[marmistrz]

@peterhuene I guess we need to first resolve #617 and #618. As for me, it's fine to merge #557 so that it passes the current version of truncation_rights and take care of the truncation semantics later on.

[kubkon]

I agree with @marmistrz that we should clarify #617 and #618. However, in the meantime, in order not to thwart progress, I'd suggest modifying the truncation_rights test case and merging this change in–it makes sense not to be able to open a file for truncation without it being opened for writing. Having said that, I would like to learn @sunfishcode's opinion on this before we make a decision.

[marmistrz]

GitHub auto-closed this issue after a force push

[marmistrz]

@kubkon the problem is that we need to support O_TRUNC even without RIGHT_FD_WRITE. RIGHT_PATH_FILESTAT_SET_SIZE + RIGHT_PATH_OPEN. We probably should just open the file for writing if O_TRUNC is present in oflags (what @peterhuene did in #557, which seems counterintuitively correct). But if we do so, then similarly to what was shown in #617 + #618, the file will implicitly receive RIGHT_FD_WRITE even when it shouldn't.

I extended the testcase to include the RIGHT_FD_WRITE but no RIGHT_PATH_FILESTAT_SET_SIZE. The old version of the change is available here: https://github.com/marmistrz/wasmtime/tree/trunc_bak
But satisfying it is currently non-trivial, because we don't have a way to specify an alternative of rights.

[github-actions]

Subscribe to Label Action

This issue or pull request has been labeled: "w", "a", "s", "i", ":", "m", "p", "l"

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

[github-actions]

Subscribe to Label Action

This issue or pull request has been labeled: "w", "a", "s", "i", ":", "m", "p", "l"

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

[alexcrichton]

I'm looking at some older PRs on Wasmtime and this is a prtty old one at this point. I'm going to close this since it's not super actionable given its age any more. Nowadays though changes like this are probably best done through the WASI proposal repositories if someone ends up coming across this again now or in the future.