win: fix EPERM when trying to write a hidden file

[sryze]

Fix an issue with writing to files marked as hidden on Windows by passing the correct file attributes to CreateFileW().

As per documentation:

If CREATE_ALWAYS and FILE_ATTRIBUTE_NORMAL are specified, CreateFile fails and sets the last error to ERROR_ACCESS_DENIED if the file exists and has the FILE_ATTRIBUTE_HIDDEN or FILE_ATTRIBUTE_SYSTEM attribute. To avoid the error, specify the same attributes as the existing file.

Fixes nodejs/node#41093

Steps to reproduce the bug:

1. Execute touch hidden-file.txt && attrib +h hidden-file.txt
2. Compile and run this program:

#include <stdio.h>
#include <string.h>
#include "uv.h"

void on_open(uv_fs_t *req);
void on_write(uv_fs_t *req);

uv_buf_t buffer;
uv_fs_t open_req;
uv_fs_t write_req;
uv_fs_t close_req;

void on_open(uv_fs_t *req) {
    if (req->result < 0) {
        fprintf(stderr, "Open error: %s\n", uv_strerror(req->result));
    }
    else {
        uv_fs_write(uv_default_loop(), &write_req, open_req.result, &buffer, 1, 0, on_write);
    }
}

void on_write(uv_fs_t *req) {
    uv_fs_req_cleanup(req);
    if (req->result < 0) {
        fprintf(stderr, "Write error: %s\n", uv_strerror(req->result));
    }
    else {
        uv_fs_close(uv_default_loop(), &close_req, open_req.result, NULL);
    }
}

int main(int argc, char **argv) {
    time_t ts = time(NULL);
    char *str;
    ssize_t result;

    if (argc < 2) {
        printf("Usage: %s <fileName>\n", argv[0]);
        return 1;
    }

    str = ctime(&ts);
    buffer = uv_buf_init(str, strlen(str));

    uv_fs_open(uv_default_loop(),
               &open_req, argv[1],
               UV_FS_O_CREAT | UV_FS_O_TRUNC | UV_FS_O_WRONLY,
               _S_IREAD | _S_IWRITE,
               on_open);
    uv_run(uv_default_loop(), UV_RUN_DEFAULT);

    return 0;
}

[bnoordhuis]

This seems like it's vulnerable to a TOCTOU bug between the GetFileAttributes() and CreateFile() calls.

Possibly it's harmless but I don't do enough Windows programming to say for sure. @libuv/windows PTAL.

[sryze]

@bnoordhuis How about this: sryze@3b17645
Is it better? It only checks for *_HIDDEN flag though. I'm not sure if it's a good idea to have 3 more CreateFile calls instead of one (+2 for possible combinations with *_SYSTEM attribute).

[sryze]

Or maybe it's better to let the caller pass a "hidden" flag from outside, like it's done for mode and flags?

[bnoordhuis]

Retrying on "access denied" seems a good way forward.

Apropos FILE_ATTRIBUTE_SYSTEM, I don't know if that's something libuv ought to be setting. Presumably the attribute is intended as a "hands off" signal.

[sryze]

OK, I've updated the code to use the 2nd approach

[sryze]

Is there anything I can improve further? Should I add some sort of flag to turn this on and leave it off by default? That way it could be enabled only by those callers who care about supporting writes to hidden files (e.g. node).

[bnoordhuis]

I can't really evaluate whether this is a good idea or not. @libuv/collaborators WDYT?

[saghul]

I'd say writing to a hidden file should just work. That said, regarding this approach can't we just set the flag always?

[vtjnash]

If you specify "w" permissions (UV_FS_O_CREAT | UV_FS_O_TRUNC), the kernel requires that the pair of flag (SYSTEM | HIDDEN) matches between the old and new files, otherwise it does not

[vtjnash]

Note that nodejs appears to explicitly document the existing behavior, so merging this would create a breaking change for nodejs, per nodejs/node#15409. Since libuv doesn't document this, it would not necessarily be a breaking change for libuv.

[Clonkex]

I just ran into this issue in NodeJS and spent a LONG time trying to understand wtf was going on. Eventually I discovered the NodeJS issue linked in the OP of this PR.

Whether or not the file is hidden should have no effect on whether it can be opened. It makes no sense, so I'm strongly in support of changing this behaviour. As far as breaking the NodeJS documentation, please do it. I'm using fs.copyFileSync, but even fs.copyFile makes no mention of this issue. In fact the only attempt to document it is a single line right at the very bottom of that page in a section describing how to use flags in fs.open, where I never would have looked in a million years because it's simply not relevant to the functions I'm using.

I hate silly behaviour like this. My opinion is that it should have been fixed years ago.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

[mominshaikhdevs]

please fix/merge this FFS.
this blocker is really really nonsensical.
Not every system is Unix & Unix Clones.

[kecskesd]

+1 for fixing this, spent an hour trying to figure out what was wrong

[vtjnash]

The problem is that this seems to be intentional design choices of your OS, and libuv generally tries to avoid working around intentional design choices of the users's OS of choice. There just does not appear to be a mapping option that can do either CREATE_NEW or TRUNCATE_EXISTING behavior, since the combined option (CREATE_ALWAYS) also adds enforcement of this secondary property.

[bnoordhuis]

In light of the above, should we close this PR? Maybe someone wants to send a documentation PR instead?

[Clonkex]

Personally I think it's utterly ridiculous that fs.CopyFile and fs.WriteFile don't work on hidden files on Windows. It's insane. There's zero reason to accept that. A documentation fix is a bandaid only. Anyone receiving access denied is going to think there's something wrong with the permissions somehow, because of course that's what that error means. It seems to me libuv is simply using the Windows API incorrectly. There's nothing "intentional" about being unable to write to hidden files.

[bnoordhuis]

Comments like yours are tedious, it's just a lot of words for "this is how I feel it should work."

A number of problems with this PR have been pointed out. Try arguing those (or better: show a way forward) instead.

[Clonkex]

A number of problems? I don't see any. The PR was updated to solve the initial problem.

@vtjnash said it appears to be an intentional design decisions of the OS. I'm saying it's not.

[bnoordhuis]

#3380 (comment) is still unsolved, AFAICT. Retry-on-error is also problematic, the file can change between calls.

edit: #3380 (comment) as well.

[vtjnash]

It looks like we could specify OPEN_ALWAYS (as the fallback, or just do it always), then immediately call SetEndOfFile on success

The NtCreateFile function also supports many more options for this (https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/ntifs/nf-ntifs-ntcreatefile), including FILE_OVERWRITE_IF, but NtCreateFile is also substantially more awkward to use

[adufswdanq]

what's the latest update on this?

bun on Windows is using libuv for I/O operations and I would like bun to respect Windows "Hidden" file attribute before it is too late, as it is for nodejs.

oven-sh/bun#6820

[sryze]

To be fair, other programming languages' standard libraries seem to suffer from the same problem on Windows, e.g. C, C++, Python, Ruby, Go - so it's not just a libuv/Node.js issue.