Ticket 480, PipeFS hangs on open for read when open for write

This is a bit of an interesting one – attempting to open a PipeFS file for reading will block if the file is already opened for writing.

Tracking down the cause took a while, running various combinations of new & old modules on RISC OS 3.7, since that seemed to be immune to the bug. And it turns out that it’s not a bug, it’s a feature (but a feature that was being hidden by a bug, and only appeared when another feature was implemented elsewhere).

What happens is that when you open a file, FileSwitch calls the filesystem’s fsfile_ReadInfo handler to see if the file exists (which is important for things like resolving references to files within image file systems). But the PipeFS fsfile_ReadInfo handler contains code which checks if the file is open for writing and then (tries to) sleep using OS_UpCall 6 until it’s no longer in that state: https://gitlab.riscosopen.org/RiscOS/Sources/FileSys/PipeFS/-/blob/PipeFS-0_24/s/PipeFS#L616 This appears to be intended to act as protection against programs which do something like call OS_File 5 to get the length of a file, allocate a block of memory, and then use OS_File 12 to load it – if that file is a pipe file which is still being written to, it’s possible that the file could have grown inbetween the two OS_File calls, causing a buffer overflow. Since typical RISC OS filesystems don’t allow the same file to be simultaneously open for both reading & writing, this isn’t something that most programs would expect to have to deal with.

Of course in this case FileSwitch doesn’t care about the size of the file, it’s only checking to see what object type it is (file/image file/directory/none). So the blocking could be avoided if FileSwitch was changed to use fsfile_ReadInfoNoLen (which doesn’t return the length, and doesn’t contain the blocking loop).

But if blocking is the intended behaviour, then why does it only block on RISC OS 5? And if you try using OS_File 5 to read the size of a pipe which is open for write, why does it claim the file doesn’t exist?

The reason for this is twofold:

1. When the OS_UpCall 6 call (made from within blockifwriting) returns, PipeFS will branch back to the 20 label in order to re-fetch the details of the file (and to double-check that the file isn’t open for write – OS_UpCall 6 implementations are allowed to return before the pollword becomes non-zero). But the FindFileOrDirectory call which it uses to get the details of the file accepts the file name in R1, and returns the internal file handle R1 (i.e. a pointer to a struct). R1 isn’t restored to its original value when returning to the start of the loop, so the second time round it’ll be called with a bogus file name, fail to find the file, and return an object_nothing response.
2. Prior to TaskWindow 0.65, calling OS_UpCall 6 from within a TaskWindow was almost always guaranteed to return before the pollword was set, because all TaskWindow did was call Wimp_Poll once to temporarily yield the task.

The object_nothing response from fsfile_ReadInfo will cause OS_File 5 and *FileInfo to report that the file doesn’t exist. But because PipeFS allows you to open files that don’t exist (fsinfo_alwaysopen flag set), FileSwitch ignores the object_nothing response when opening the file for read, allowing BASIC OPENIN or similar to succeed.

On newer OS versions with TaskWindow 0.65+, the better implementation of OS_UpCall 6 means that instead of returning immediately, blockifwriting will (usually) wait until the file is no longer open for writing. So OS_File 5, *FileInfo, and OPENIN will all block. But when they do return, there’s still the register corruption bug to contend with – so OS_File 5 & *FileInfo will still claim the file doesn’t exist.

Fixing the register corruption bug in PipeFS will make it so that everything works “correctly”, regardless of TaskWindow version – OS_File 5, *FileInfo, and OPENIN will all block until the file is closed for writing, and OS_File 5 & *FileInfo will now correctly report the file info instead of claiming it doesn’t exist.

Of course it’s still not ideal that OPENIN blocks, but as mentioned above, that could probably be fixed by changing FileSwitch to use fsfile_ReadInfoNoLen.

But then there’s OS_File 5, which will now block instead of claiming the file doesn’t exist. Is anyone aware of any software which could break because of this? (e.g. if the same thread that opened the file for writing tries to read the properties of the file it’ll deadlock itself)

And similarly, would anything break if we removed the blockifwriting logic from fsfile_ReadInfo? After all, the “file not found” response means anyone who tried using OS_File 5+12 to load from PipeFS would have quickly discovered that it only works if the writer closes the file before handing it over to the reader (in which case it shouldn’t matter that the behaviour of OS_File 5 has changed for files which are open for write).

It’s also interesting that the PRMs suggest using OS_GBPB 10 to read the length of PipeFS files. Is that because the person writing the documentation is expecting OS_File 5 to block, or is it because they’d observed that OS_File 5 sometimes claims that files don’t exist? Note that the code which implements OS_GBPB 9-12 doesn’t contain any blockifwriting logic, so a program which wants to use OS_File 12 could still crash itself with a buffer overflow if it was to combine OS_GBPB 10 with OS_File 12, lending more weight to the idea that software which is reading from PipeFS will have been written correctly (either waiting for the writer to close it before using OS_File 12 to load it in one go, or using OPENIN/etc. to read it in a piecemeal fashion)

Tracking down the cause took a while

You’re up at 2am, so that’s dedication!

Of course it’s still not ideal that OPENIN blocks, but as mentioned above, that could probably be fixed by changing FileSwitch to use fsfile_ReadInfoNoLen.

That reason code was added by SKS according to the change log in HdrSrc, perhaps Stuart can add insight? The PRM mentions its use with NetFS to avoid having to call the file server, whether that’s a speed optimisation to avoid asking, or a special just for *Copy to avoid the OS_File 5 then 12 problem I’m not sure.

But then there’s OS_File 5, which will now block instead of claiming the file doesn’t exist. Is anyone aware of any software which could break because of this? (e.g. if the same thread that opened the file for writing tries to read the properties of the file it’ll deadlock itself)

I think if I did an OS_File 5 on a file that didn’t exist in PipeFS I’d expect that to return that the file didn’t exist, as it’s not in the catalogue. If the file is open for writing though, I would expect it to block until closed. I think that’s what you’re proposing.

A quick search shows various of the Configure plugins, AcornSSL, perl, Env, (TaskWindow/PipeFS) all use PipeFS if you need a guinea pig.

Looks like I added fsfile_ReadInfoNoLen specifically to speed up *Copy on NetFS, so that when testing whether the destination file existed the fileserver didn’t have to expensively calculate the file length to return. I think it might not have stored that info in old JesMap days and have to walk the blocks; possibly a feature in parallel development: ‘when FileServer feels up to it’. There are a couple of other places that might have benefited from that.

Dunno about PipeFS, after my time. I have memories of people BITD mentioning it as something one customer wanted and was implemented so that particular use case worked. I would imagine that was to have to writer start before the reader.

You’re up at 2am, so that’s dedication!

No, that’s more just a sign that it took longer than expected to translate thoughts to words.

I think if I did an OS_File 5 on a file that didn’t exist in PipeFS I’d expect that to return that the file didn’t exist, as it’s not in the catalogue. If the file is open for writing though, I would expect it to block until closed. I think that’s what you’re proposing.

That’s what will happen if I fix the fsfile_ReadInfo handler, yes.

For OS_File 5 of a file that’s open for writing, we have the following behaviours:

And for OPENIN:

I think it might not have stored that info in old JesMap days

I have the following comment in my FS0 reader notes:

For file length, you need to jump to the map information for
the object and read the length from there...
  File size is ( ?8 + ( ?13 * 256) + (?14 * 64K) )
  ?8  = LSB of object length     (the rest)
  ?13 = LSB of contiguous blocks (256 byte blocks)
  ?14 = MSB of contiguous blocks (64K blocks)
  And for non-continuous blocks?

So it looks like the FileStore has to do at least two reads to determine the size of a file (the directory, then the map entry prefixing the file), possibly more depending on whether or not files are always kept in contiguous sectors. I didn’t think the FileStore did auto-compaction, but a quick look at the ROM dump doesn’t give any indication of a command like *FSCompact…

Jeffrey observed

call OS_File 5 to get the length of a file, allocate a block of memory, and then use OS_File 12 to load it

But then claimed

Since typical RISC OS filesystems don’t allow the same file to be simultaneously open for both reading & writing, this isn’t something that most programs would expect to have to deal with.

No, this pattern has always been faulty and is nothing to do with open files. Files can easily change size on networked and hosted filing systems, but even a stand-alone RO may have tickers, callbacks, vector claimants or event handlers that log to a file or otherwise open-append-close in the background.

This pattern is fixed by the SaferOSFile module which adds an optional buffer length to the OS_File,12/14/16/255 API, and I’d encourage people to always use the safe version of those APIs as it’s 100% backwards compatible.

I thought the whole idea of a pipe filing system was to allow simultaneous reads and writes? Well, if not the whole idea, at least a valuable feature.

If you don’t allow simultaneous reads and writes, then you might just as well write a file, close it, and read it; which you can do on any filing system.

This pattern is fixed by the SaferOSFile module which adds an optional buffer length to the OS_File,12/14/16/255 API, and I’d encourage people to always use the safe version of those APIs as it’s 100% backwards compatible.

Sounds like the way the OS ought to be

Making FileSwitch use fsfile_ReadInfoNoLen wasn’t too hard, and nothing exploded during testing, so there are now a couple of MRs open to fix the different issues. This should result in OS_File 5 blocking and OPENIN not blocking when they’re used on pipe files which are currently opened for write.

https://gitlab.riscosopen.org/RiscOS/Sources/FileSys/FileSwitch/-/merge_requests/14
https://gitlab.riscosopen.org/RiscOS/Sources/FileSys/PipeFS/-/merge_requests/1

No, this pattern has always been faulty

Yes I was aware of that, I just neglected to mention it.

Files can easily change size on networked and hosted filing systems, but even a stand-alone RO may have tickers, callbacks, vector claimants or event handlers that log to a file or otherwise open-append-close in the background.

I think the window of opportunity is vanishingly small – it’s the duration between reading the file size, allocating the memory, and trying to load the file… but it’s possible.

I’m pretty sure I once ran into an issue where a file was changed on the server but NetFS returned the (smaller) size that it had just recently read from the server, not the new size. Was this RISC OS 2, perhaps? The vanishingly small window of opportunity is rather larger when it’s a slow floppy-based server with multiple clients trying to do stuff at the same time. ;-)

Sounds like the way the OS ought to be

Yeah, it’s a bit mad not to ask for and respect a buffer size. This is why I always load data with HeeBeeGeeBee. It’s not any safer in things changing in the background, but one gives it a count of how many bytes to read so there’s no risk of overrun.

Anything that can happen, will happen. Anythnig bad, anyway.

Anything that can happen, will happen. Anythnig bad, anyway.

Just with sufficiently low frequency that you never track it down. I recall there being a bug in the Cambridge Ring that would eventually stiff systems after six months. So you just had to remember to reboot every couple of months…

Rick remarked

HeeBeeGeeBee […] so there’s no risk of overrun

Indeed, buffer overrun is a real risk, and SaferOSFile uses that method internally of course.

And the reason that module exists is I did actually hit the problem on a hosted system.

the window of opportunity is vanishingly small

‘Small’ is a relative term. Compared with your computer, your ability to add up is ‘small’. Given a Basic program in a TaskWindow doing OS_File,5, displaying the results, DIMing a buffer and OS_File,255ing the data, there’s plenty of opportunity for change even on a stand-alone RO.

Stewart suggested

with sufficiently low frequency

Which reminds me… the next Bad Time To Save is between 05:22:34 08-Nov-2023 and 08:17:17 08-Nov-2023, which is sufficiently early that it’s unlikely to affect anyone.

[BTTS: There is a very small possibility that a file saved during such a period will lose its filetype. This is caused by a combination of coincidence and nostalgia.]

The idea that computers suffer from nostalgia is an interesting one, albeit clearly real.

The idea that computers suffer from nostalgia

Sometimes they do unwanted things just to get you wound up, ask any IT support bod.

nostalgia

In this case a codified but not clearly documented veneration of the ancients.

My version of Filer tries to be a lot more intelligent about unstamped but identifiable files for the sake of !65Host and !BeebIt, but results are mixed – the DTP protocol is unclear (surprise!) about the filetype at +&40 and what the recipient should do when it does not match that of the file itself (which is a perfectly normal function – e.g. shift-double-click in Filer). The lie in the message should cause the file to be loaded, but should not be used as the filetype thereafter.

which is sufficiently early that it’s unlikely to affect anyone.

;)

Except those of us who run servers and periodic logging.

Why is it’s bad time to save? We can’t be flipping on the high bit as it’s a period of about three hours. There’s nothing marked in my astronomical diary for that day. And it’s National Cappuccino Day in America.

Hmm…?

The lie in the message

There’s your problem right there. If the Filer sends a message with a filleule, it’s entirely reasonable for the receiver to treat that as correct.

What the Filer should have done is have a flag bit to say “load this, don’t run it”, rather than blatant lying to fudge the desired behaviour.

why?

Coincidence and nostalgia: Timestamps and FileType are stored in the Load and Execution address metadata. Twelve bits must be set, the FileType is 12 bits, and the remaining 40 bits are the timestamp in cs. At unfortunate moments the least significant 32bits of the timestamp can match the combination of set bits, filetype and most significant byte of the timestamp, and hence Load=Exec.

However, for historical reasons if the Load=Exec then the OS (by which I mean FileSwitch and Filer) regard the file as unstamped. Bad luck!

Significantly, nothing tries to avoid that outcome by incrementing the cs – it just goes ahead and produces unusable metadata. Sigh.

Rick mangled

There’s your problem right there. If the Filer sends a message with a filleule, it’s entirely reasonable for the receiver to treat that as correct.

Assuming you meant it’s reasonable for senders of DataLoad and DataOpen to lie about the filetype, you are correct, though you’d be hard pressed to find any documentation that states that.

What the Filer should have done is have a flag bit to say “load this, don’t run it”, rather than blatant lying to fudge the desired behaviour.

DataOpen means ‘run’ in this sense, and DataLoad means ‘load’.

Another pertinent point that is not generally considered is there is significant semantic difference between a Broadcast DataOpen (say) and one that has been sent direct to the app in question. There is an argument that in some cases an app might ignore the former but must always honour the latter.

In reality there’s a lot to be desired in the DTP, which is why my Filer uses an additional DTP message – DataCheck – and many of my apps have individually-configurable filetype affiliation:

Wonders what Panos thinks of ‘stamped’ files with the top 24 bits of load/exec set, but Load=Exec.

Didn’t Panos rely on file extensions, effectively? I forget the details.

(i.e. that’s where the blasted “c.foobar” comes from, due to the large size of ADFS directories)

I wanted timestamped files in Arthur (and on the Master – see Edit), so just nicked the 5-byte cs time in load/exec idea from Panos, keeping the top 12 (20) bits set for vague compatibility – strictly Panos required the top 24 bits set. I think Panos would still recognise files set with I/O processor addresses and load them in there.

Loved Edit. Used Twin throughout Arthur because of Edit.

I/O

So I/O Fxxx load addresses would be vulnerable if there were any RAM above E000… which fortunately there wasn’t.

Random bit of fun M128 trivia: Not only could the 6845 use the ‘shadow RAM’ from 3000-7FFF, it could also access the ‘MOS RAM’ at 8000-8FFF when paged in (addresses 2000-2FFF mapped there). A 640×304 ‘MODE0’ might have been possible.