Handling of read-only files and a file descriptor leak

[SteveFosdick]

I have a filing system test program (see https://github.com/SteveFosdick/FsTest). One of the things it does is open a file for reading (OPENIN, i.e. OSFIND A=&40) and then attempt to write to it to check that an error is generated. The error that aund is sending back is 0xff/Bad file descriptor.

Looking at the code this is because aund correctly heeds the read-only attribute in the open request and opens the file read-only at the Unix level and then, when it receives a request to write to it, it calls write(2) which fails and the Unix errno EBADF is not mapped to an error message so it returns 0xff and the result of strerror(3).

Now there is code to set a handle attribute can_write but this only inspects the Acorn-specific file permissions and does not seem to be affected by whether the client requested the file to be open for writing. Is that correct? Do you think aund should set can_write to false if the original request specified read only?

Finally, while looking at fs_open to see what was going on, it looks like the file descriptor being used to open the file read-only as a "does it exist" test (line 112) is leaked as I can't see it being closed.

[SteveFosdick]

Looking at this code in more detail it also seems that fs_open, c->client->handles[h]->can_write is never being initialised to false, either explicly, or by a memset of the whole structure to zero.

If I change this part of fs_open to:

    c->client->handles[h]->can_write = false;
    c->client->handles[h]->can_read  = false;
    c->client->handles[h]->is_locked = false;
    ftsp = fts_open(path_argv, FTS_LOGICAL, NULL);
    f = fts_read(ftsp);
    if (f->fts_statp->st_mode & S_IWUSR && !request->read_only) {
        c->client->handles[h]->can_write = true;
    }
    if (f->fts_statp->st_mode & S_IWOTH && !request->read_only) {
        c->client->handles[h]->can_write = true;
    }
    if (f->fts_statp->st_mode & S_IRUSR) {
        c->client->handles[h]->can_read = true;
    }
    if (f->fts_statp->st_mode & S_IROTH) {
        c->client->handles[h]->can_read = true;
    }
    if (f->fts_statp->st_mode & S_IXUSR) {
        c->client->handles[h]->is_locked = true;
    }

It behaves as I would expect. I also looks like the version of NFS I have is throw by the 255 error code - once it is a smaller value it accepts it with no timeout issues.

[sai2791]

I am not seeing this from within BeebEm, Master with NFS 4.25. In basic 10A=OPENIN "TEST" 20FOR F=1 TO 10 30BPUT #A, RND(256) 40NEXT F 50CLOSE #A.

If the file does not exist I get Net channel 0 at line 30 if the file exists I get Not open for update on channel 35 at line 30

I will try the same code using bbc mode and NFS 3.60

[sai2791]

in bbc b mode with NFS 3.60 with no file the client hangs with an existing file, I get Bad file descriptor at line 30

I think it would be better to have the check when I write the bytes, let me see what I can hack together

[SteveFosdick]

I am not seeing this from within BeebEm, Master with NFS 4.25. In basic 10A=OPENIN "TEST" 20FOR F=1 TO 10 30BPUT #A, RND(256) 40NEXT F 50CLOSE #A.

If the file does not exist I get Net channel 0 at line 30 if the file exists I get Not open for update on channel 35 at line 30

I think these error must be being generated in the client as neither match what I am seeing over the network with the 3.62 client (BBC B).

[SteveFosdick]

I have put in a pull request. There are separate commits as there are a number of separate but related things in this issue.

[SteveFosdick]

in bbc b mode with NFS 3.60 with no file the client hangs

Yes, this matches I was seeing - it's because aund doesn't reply when send an invalid (not open) channel number.

with an existing file, I get Bad file descriptor at line 30

Yes, that's the error from Linux from attempting a write on a file opened O_RDONLY. If you're tests are working with this then I may need to re-examine it. I think it would still be better to have an Acorn error for this rather than forwarding the error string from Linux which isn't very helpful (even on Linux).

I think it would be better to have the check when I write the bytes, let me see what I can hack together

That what https://github.com/SteveFosdick/aund/commit/4e139468dc6c473a06be6b8339eb366a9494a7b7 does.

[sai2791]

I've modified your changes a little after the pull, it basically checks if the file should exist before opening it, the code also checks if the handle is read only, and if so gives and appropriate error not 0xff as before.

[sai2791]

Checked emulating a master with NFS 4.25 and BBC B with NFS 3.60