From e7741d18537794349c7cc47129ef4f1165202822 Mon Sep 17 00:00:00 2001 From: sunrisepeak Date: Mon, 21 Sep 2026 11:47:36 +0800 Subject: [PATCH] a file opened for writing could be written and not described MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `kal_fs_file_info` answers through NtQueryInformationFile with FileBasicInformation, which requires FILE_READ_ATTRIBUTES on the handle. `FILE_GENERIC_WRITE` does not carry it — winnt.h defines it as STANDARD_RIGHTS_WRITE | FILE_WRITE_DATA | FILE_WRITE_ATTRIBUTES | FILE_WRITE_EA | FILE_APPEND_DATA | SYNCHRONIZE — so a file opened with KAL_OPEN_WRITE and nothing else was granted every right except the one needed to ask it about itself. Through musl that surfaces as a descriptor refusing its own `fstat` a line after it was opened: open(..., O_WRONLY|O_CREAT|O_TRUNC) -> fd=3 fstat(3) -> -1, EACCES measured on a Windows runner. libarchive does exactly that pair when opening an archive for output (archive_write_open_filename.c:178 and :189), and reports `Couldn't stat ''` — a message that names the verb and interpolates the argument. Four separate investigations went to the argument: whether the path lay outside the program's preopened directories, whether two temp-directory helpers disagreed about its 8.3 short form, whether the 8.3 component failed to resolve, and whether the errno for a missing name was wrong. Each was refuted, and the path had already succeeded by the time the failure happened. FILE_READ_ATTRIBUTES is now requested always. It is the minimal right for the question — metadata and no data — and `kal_fs_info` already asked for it by itself, so this file knew the requirement in one place and not the other. The no-flags case keeps its previous meaning explicitly rather than by falling out of `access == 0`. The criterion is `tests/write_handle_answers_info.cpp`. It states in its header that Wine does not enforce the access check and answers rc=0 either way, because "it passes under Wine" was offered as evidence three times while this was open, and an environment that cannot produce the failing condition is not a second opinion about it. --- src/fs.cpp | 34 +++++++++- tests/write_handle_answers_info.cpp | 98 +++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 2 deletions(-) create mode 100644 tests/write_handle_answers_info.cpp diff --git a/src/fs.cpp b/src/fs.cpp index b067d0f..33e07ac 100644 --- a/src/fs.cpp +++ b/src/fs.cpp @@ -281,7 +281,34 @@ int kal_fs_open(kal_dir base, const char* name, kal_uintptr len, void* root = dir_handle(base); if (!root || out == nullptr || !okw::acceptable(name, len)) return kal_err_invalid; - unsigned long access = 0; + // FILE_READ_ATTRIBUTES ALWAYS, BECAUSE ASKING A FILE ABOUT ITSELF IS NOT + // READING IT. + // + // `FILE_GENERIC_WRITE` carries FILE_WRITE_ATTRIBUTES and not its + // counterpart (winnt.h: STANDARD_RIGHTS_WRITE | FILE_WRITE_DATA | + // FILE_WRITE_ATTRIBUTES | FILE_WRITE_EA | FILE_APPEND_DATA | SYNCHRONIZE), + // while `kal_fs_file_info` answers through + // `NtQueryInformationFile(FileBasicInformation)`, which requires + // FILE_READ_ATTRIBUTES. A file opened for writing alone could therefore be + // written and not described: + // + // open(..., O_WRONLY|O_CREAT|O_TRUNC) -> fd=3 + // fstat(3) -> -1, EACCES + // + // measured on a Windows runner through musl's `do_fstat`, where the + // descriptor is valid by construction and there is nothing to refuse. + // libarchive does exactly that pair when opening an archive for output + // (archive_write_open_filename.c:178 and :189) and reports + // `Couldn't stat ''`, which names the verb and interpolates the + // argument --- and sent four investigations to the argument. + // + // The right is the minimal one for the question: it grants metadata and no + // data. `kal_fs_info_by_name` below already asks for it by itself, so this + // file knew the requirement in one place and not the other. + // + // Wine does not enforce the check, so it answers `rc=0` either way. That + // is why it is not a second opinion about this. + unsigned long access = FILE_READ_ATTRIBUTES; if (flags & KAL_OPEN_READ) access |= FILE_GENERIC_READ; if (flags & KAL_OPEN_WRITE) access |= FILE_GENERIC_WRITE; if (flags & KAL_OPEN_APPEND) { @@ -293,7 +320,10 @@ int kal_fs_open(kal_dir base, const char* name, kal_uintptr len, access &= ~static_cast(FILE_WRITE_DATA); access |= FILE_APPEND_DATA; } - if (access == 0) access = FILE_GENERIC_READ; + // Neither READ nor WRITE asked for: the caller wants the node, not its + // contents. FILE_READ_ATTRIBUTES above is already the whole of that, and + // widening it to FILE_GENERIC_READ would grant data nobody requested. + if (access == FILE_READ_ATTRIBUTES) access |= FILE_GENERIC_READ; // The whole of the intent, expressed once. Clause 7.8: an open followed by // a truncation is two operations, and a program that stopped between them diff --git a/tests/write_handle_answers_info.cpp b/tests/write_handle_answers_info.cpp new file mode 100644 index 0000000..b0d418b --- /dev/null +++ b/tests/write_handle_answers_info.cpp @@ -0,0 +1,98 @@ +// A regression test: a file opened for writing alone can still be described. +// +// `kal_fs_file_info` answers through NtQueryInformationFile with +// FileBasicInformation, which requires FILE_READ_ATTRIBUTES on the handle. +// `FILE_GENERIC_WRITE` does not carry that right -- winnt.h defines it as +// STANDARD_RIGHTS_WRITE | FILE_WRITE_DATA | FILE_WRITE_ATTRIBUTES | +// FILE_WRITE_EA | FILE_APPEND_DATA | SYNCHRONIZE -- so before the fix a file +// opened with KAL_OPEN_WRITE and nothing else could be written and not +// described, and the query came back kal_err_access. +// +// Through musl that surfaces as a descriptor the caller opened one line +// earlier refusing its own `fstat`: +// +// open(..., O_WRONLY|O_CREAT|O_TRUNC) -> fd=3 +// fstat(3) -> -1, EACCES +// +// which is what libarchive does when it opens an archive for output +// (archive_write_open_filename.c:178 and :189). Its message, `Couldn't stat +// ''`, names the verb and interpolates the argument; four separate +// investigations went to the argument before anyone read the two lines. +// +// WINE DOES NOT ENFORCE THE ACCESS CHECK and answers rc=0 either way, so this +// test only reports on Windows. That is stated here because "it passes under +// Wine" was offered as evidence three times while this was open, and an +// environment that cannot produce the failing condition is not a second +// opinion about it. +#include +#include +#include + +namespace { + +int failures = 0; + +void check(bool ok, const char* what) { + if (!ok) { + std::printf(" FAILED %s\n", what); + ++failures; + } +} + +} // namespace + +int main() { + // The first preopened directory, the same way handle_inheritance.cpp finds + // one: a test writes where the program was given somewhere to write. + kal_dir cwd{}; + bool haveDir = false; + { + const kal_uintptr n = kal_fs_preopen_count(); + for (kal_uintptr i = 0; i < n && !haveDir; ++i) { + kal_dir d{}; + char nm[1024]; + kal_uintptr len = 0; + if (kal_fs_preopen(i, &d, nm, sizeof nm, &len) != kal_ok) continue; + cwd = d; + haveDir = true; + } + } + if (!haveDir) { + std::printf("openkal-windows: no preopened directory to test in\n"); + return 1; + } + + static const char name[] = "okw-write-handle-info.tmp"; + const kal_uintptr len = sizeof name - 1; + + kal_file f{}; + const int opened = kal_fs_open(cwd, name, len, + KAL_OPEN_WRITE | KAL_OPEN_CREATE | KAL_OPEN_TRUNCATE, + &f); + check(opened == kal_ok, "a file opens for writing"); + if (opened != kal_ok) { + std::printf("openkal-windows: kal_fs_open returned %d\n", opened); + return 1; + } + + // THE ASSERTION. The handle was granted a line ago and the caller asks it + // about itself; there is nothing here for the object manager to refuse. + struct kal_node_info info = { }; + info.self_size = sizeof info; + const int described = kal_fs_file_info(f, KAL_INFO_ALL, &info); + std::printf(" kal_fs_file_info on a write-only handle -> %d\n", described); + check(described == kal_ok, "a write-only handle answers kal_fs_file_info"); + + // And the answer is the file's, not a zeroed structure: nothing has been + // written, so the length is nought and the kind is a regular file. + if (described == kal_ok) { + check((info.present & KAL_INFO_KIND) != 0u, "the answer states the kind"); + check(info.kind == kal_node_file, "the kind is a regular file"); + } + + kal_fs_close_file(f); + kal_fs_remove(cwd, name, len); + + std::printf("openkal-windows: a write-only handle answers for itself\n"); + return failures == 0 ? 0 : 1; +}