No notification given when DeviceFS streams/buffers are being closed

That memory corruption I found a couple of weeks ago was down to *shutdown closing EtherUSB’s DeviceFS streams without EtherUSB knowing about it. The next time EtherUSB would use the direct buffer manager calls to write data to a buffer it would end up corrupting RMA.

After looking around for a bit, I’ve determined that the only notification that’s given when a device FS stream is closed is upcall 11. Trouble is, this upcall happens once the stream and associated buffer has already been destroyed – so is a bit useless for anything that needs to interact with the device/buffer from the background.

For the moment I’ve fixed this in EtherUSB by making it sit on FSControlV & FindV so it can detect when EtherUSB-owned DeviceFS streams are being closed. But this is far from ideal, particularly because it’ll only work properly if EtherUSB gets the vector call before FileSwitch. Which means EtherUSB also needs to sit on Service_Reset, because FileSwitch will reclaim the vectors on that service call, thus promoting it to the front of the claimant queue.

What are peoples thoughts on a better fix for the issue? A ‘DeviceFS stream closing’ upcall would be one of the obvious choices. Perhaps some kind of notification for when buffers get destroyed too. I’m also considering adding markers to buffer maanger’s internal structures so it can detect attempts to interact with dead buffers via the direct calls. It might also be worth adding a generic upcall/event for files being closed, for the benefit of any other filing systems that provide users with low-level access.

Also the fact that all open device FS streams get closed without warning when *shutdown is issued points to a fairly major flaw in the USB stack (yet again) – any device drivers which are using the device FS interface (i.e. everything except the keyboard/mouse/hub drivers which are built into USBDriver itself) will find themselves suddenly unable to communicate with their devices, and with any outstanding requests cancelled. For networking this may not be such a serious issue, but for filesystems (I haven’t checked how SCSISoftUSB deals with this), USB monitors (should we ever support them), custom input devices, etc. it’s not so great.

What are peoples thoughts on a better fix for the issue?

In the specific case of the shutdown dialogue I was going to suggest Service_ShutdownComplete might be simpler, however I see you turned over the stone and some woodlice escaped.

Certainly seems pointless having every DeviceFS USB client jump through hoops or sitting on FindV, especially if the order of registration is important.

Does this mean typing *SHUT might cause bad things to happen? The DeviceFS stream closes then SCSIFS tries to write back (in response to the shut) and explodes. I wonder if that’s why InetSetup never managed to reset properly (until recently, when it was changed to issue TaskManager_Shutdown instead, perhaps masking the true problem?).

Does Service_CloseFile help? Probably not, it doesn’t look like it’s possible to refuse to close a file via that service call.

Yeah – sounds like extending DeviceFS in the manner suggested is required.

Does Service_CloseFile help? Probably not, it doesn’t look like it’s possible to refuse to close a file via that service call.

Service_CloseFile appears to be for use with image filing systems; from the brief look I had at it, it looks like it gets issued when an attempt is made to open an image file as a file. I.e. it’s used to dismount the image before it gets poked with.

The perennial problem of vector claimant ordering is one I hit in 2002 and is why !!DeepKeys has its unusual official name – the double pling a pathetic attempt to get run as early as possible in Boot$ToBeLoaded.

The solution was VectorExtend, which uses the top eight bits of the vector number as a signed claimant prioritisation field. This solves the chronological ordering problem (amongst other things).

I have not yet written an RO5 version as it does rather more than vector claimant prioritisation and as such is rather complicated. However I’ve just purchased the DDE and I’m working on it.

It might also be worth adding a generic upcall/event for files being closed, for the benefit of any other filing systems that provide users with low-level access.

Thinking about this some more, we even need this for programs which stick to the regular FS APIs. RISC OS only uses a small pool of file handles, and has a habit of reusing the most recently freed one. Upshot being that if your file gets closed without you knowing about it, and then someone else opens a new file, you’ll end up fiddling with their file instead of yours.

Well an Upcall won’t help existing programs. Perhaps FileSwitch should always cycle through all 255 handles to guard against this issue.

Well an Upcall won’t help existing programs.

Depends; anything that uses the OS SWIs directly won’t be helped, but it should be possible to update the shared C library, unixlib, etc. to watch for the upcall and mark the appropriate FILE as closed.

That memory corruption I found a couple of weeks ago was down to *shutdown closing EtherUSB’s DeviceFS streams without EtherUSB knowing about it. The next time EtherUSB would use the direct buffer manager calls to write data to a buffer it would end up corrupting RMA.

I don’t really know how EtherUSB works, so I could be quite wrong about this, but the fact that you say EtherUSB uses the Buffer Manager calls directly implies that it knows the buffer handles that its DeviceFS stream(s) use(s). If so, could it not attach an “owner change routine” (PRM 4-97) that gets called (quoting):

• when an attempt is made to remove or deregister the buffer by calling Buffer_Remove or Buffer_Deregister respectively

or

• when an attempt is made to kill the buffer manager

and realise at that point that the stream is being killed underneath it? Maybe this wouldn’t work because there are other scenarios in which a stream’s buffers get killed? I don’t know, but thought it was worth mentioning. Sorry if I’m miles off the point here.

anything that uses the OS SWIs directly won’t be helped, but it should be possible to update the shared C library, unixlib, etc. to watch for the upcall and mark the appropriate FILE as closed.

Oh indeed, I do think an UpCall is a good idea, but I was pointing out that the preference for reusing handles causes or exacerbates the problem and that could also be changed, helping older programs directly (by causing them to fail in the right way). I suspect FontManager’s resilience disguises the problem – most open files belong to it).

Strictly speaking file handles aren’t bytes in RISC OS. Although there are something like four OS_Bytes which take file handle parameters, I can only think of two that have no equivalent 32bit call – changing the EXEC and SPOOL handles (or maybe I’ve forgotten what their proper versions are… if there aren’t any, there should be).

I’m tempted to write a module to push all file handles into a much higher range just to see what breaks. Enforce 32bit file handles and you could avoid ever reusing a closed handle (for some definition of “ever”).

I’m tempted to write a module to push all file handles into a much higher range just to see what breaks

Any BASIC programme that stores the file handle in a real variable rather than integer?

Any BASIC programme

BASIC reals are 40bit for a reason.

but the fact that you say EtherUSB uses the Buffer Manager calls directly implies that it knows the buffer handles that its DeviceFS stream(s) use(s).

Correct. There are some hoops you can jump through to get at the relevant information.

If so, could it not attach an “owner change routine” (PRM 4-97)

Interesting – I must have missed that one. It looks like DeviceFS already uses it to claim the buffer, and issues Service_DeviceFSCloseRequest when it gets called (despite all the documentation suggesting that DeviceFSCloseRequest only gets used when an attempt is made to open a stream on a device that’s ran out of free streams).

Unfortunately, by the time the buffer is being deleted, any outstanding USB transfers will have already been cancelled, and (I think) USBDriver will have already be in the middle of closing the associated USB pipe. So we really do need something that happens inside DeviceFS before anything starts being shutdown.

I’m tempted to write a module to push all file handles into a much higher range just to see what breaks.

Given that the range has always been stated as counting down from 255; if something should break I’d be tempted to consider your mods the odd one out. That said, I think all my code uses a word for file handles (quicker to access at the cost of three bytes).

Would it not be possible to change whatever allocates handles to exhaust all handles before recycling? In other words, keep counting down to one before wrapping back?

Given that the range has always been stated as counting down from 255

I don’t recall ever seeing that statement. On the contrary, I remember seeing a reminder (though, sadly, I don’t remember where) that these handles are not restricted to 8 bits.

Would it not be possible to change whatever allocates handles to exhaust all handles before recycling? In other words, keep counting down to one before wrapping back?

Yes. Which is why I suggested:

Well an Upcall won’t help existing programs. Perhaps FileSwitch should always cycle through all 255 handles to guard against this issue.

:-)

If file handles are allowed to go over 255, it would be nice to provide some way of enumerating all open files. Currently, testing every handle from 1 – 255 suffices, but obviously that won’t be ideal with a larger range of handles.

You mean like this? :)

https://www.riscosopen.org/wiki/documentation/show/OS_FSControl%2058

Goodness me, you learn something new every day. That’s new.

Well look at that :)