robhagemans / pcbasic

PC-BASIC - A free, cross-platform emulator for the GW-BASIC family of interpreters
http://www.pc-basic.org
Other
393 stars 48 forks source link

Add ability to selectively disable writes to devices #188

Closed rbergen closed 2 years ago

rbergen commented 2 years ago

This is the implementation of the suggestion I made in #186. It allows the following write features to be switched on or off, independently:

Configuration-wise, the user can choose to disable specific writes; by default all are enabled. In the Sessions API, the list of write operations that are enabled are specified; the default here is that all are disabled. This was done to explicitly allow writes internally, while making the current behaviour the default. If writes are attempted of a type that are not allowed, a "Device I/O error" will be raised within PC-BASIC; this seems to be the only suitable I/O related error that is generic across devices. As such, it is the I/O error that all BASIC programs should be able to process, in principle.

Aside from the implementation of the feature itself:

All in all, I think the feature is implemented consistently, both in a functional sense and at the source code level. Getting it to work proved to be the most challenging for the cassette device. The reason is that it took me a while to figure out the relationship between cassette files, the cassette stream and the underlying format streams.

In the interest of stating the obvious: the fact I'm opening a PR means that I believe this suggestion is now ready for review, not that it's free of flaws. :)

robhagemans commented 2 years ago

I'm still struggling with the 'why' of this - why first mount a device to then disable writing to it? It just seems confusing to me. The mechanism in PC-BASIC to regulate access to devices is though the --mount option (and device options like --lpt1) and this introduces a separate and partly conflicting mechanism.

Do you have an example of how this would be used? In #186 you mention running external programs that are not vetted, though we also agreed that this should not be seen as a security feature. One could equally run such a program with no mounts, and it would have largely the same effect, though it would also block reading data files. The program itself could be loaded from a filesystem location with --load, without requiring a mount.

I think there is an issue in PC-BASIC with being able to override defaults for list options: right now it seems not possible to override the config to have no mounts. But I think the right thing to do would be to fix that, not work around it with an additional option.

There may also be a case for having the possibility of read-only mounts, though I'd also suggest you can just mount to a work directory only and copy stuff over there, so that overwrites won't cause a problem. Having an additional command-line option and a new parameter for the session seems very heavy-handed for the purpose... Particularly as one of my priorities has been to reduce the number of API parameters and command-line options; once an option is there and people start using it it becomes quite tedious to manage any change to it.

I'd have some other observations - the change to the Session API seems not backward compatible, it's confusing to open a session with an explicit mount to some device and then not be able to write because you didn't also enable writes to that device - but I think this is secondary to the more fundamental point of having two competing mechanisms to regulate device access.

As an aside, I'd also like to avoid touching the serial port implementation at all as the test coverage is poor, I don't have a serial port setup to try it, it's hell to debug because there are too many moving parts to a serial connection, and there are some people around who really depend on it working.

rbergen commented 2 years ago

I think the right approach here is to start with the most fundamental question first, which is if you see any point in adding read-only mounts in some shape. Personally, I like the idea of being able to let PC-BASIC work with whatever is available on my system, as long as I know it can't make changes or communicate out what it reads. But it doesn't matter what I like, what matters is what point you see in it, if any. I'm blissfully unaware of all the communication you've had about PC-BASIC's use with its userbase, and I also don't know what development path you see for it. Because of that, I can just make a change like this based on my personal preferences (the fact I enjoy coding also helps) and throw it at the wall. It is very much up to you to decide what sticks, and I will accept any decision you make about that up-front. If you can see a "why" that does make sense, then your ideas about it and the future of PC-BASIC would then direct the "what" and the "how" in a rather straightforward manner. The current implementation is the one with the most straightforward configuration (from an implementation perspective) that is the least invasive one from a regular user's perspective. And it's not by accident that I chose the "lazy" approach concerning configuration, after you've already expressed your doubts in #186.

Some specific comments in response to your remark:

Just to be clear: if you decide to not merge this and possibly never implement anything read-only then I won't be offended in any way. Nobody asked me to do this, and I enjoyed implementing it as a thing in itself. If nothing else, I now understand the workings of the I/O part of PC-BASIC a lot better than I did before. I will use this change myself regardless, but it can also just live in my fork; it surely won't do much damage there.

robhagemans commented 2 years ago

That's fair, and I much appreciate your contributions and active interaction with this codebase, it's just that this feature goes against my intuitions of how the code should develop. I realise that since I don't have a public roadmap there's no way for people to know that, and my ideas may not be fully developed in the first place.

I added some observations on the kind of things I would want to work through if I were going to consider merging the PR, but what I want to avoid is having a lot of iterative updates to this PR based on my detailed feedback - only for me to then turn around and say, actually, I didn't like the concept of a 'no writes' switch to start with so tough luck.

As to security guarantees, I think we just have different views here. My sense is this is something like making people pay for parking in an enclosed area with a guard and then putting up a small sign that you take no liability for thefts. It doesn't work, as everything else gave the impression that you were guarding their car. The same admittedly applies to a read-only mount.

rbergen commented 2 years ago

That's fair, and I much appreciate your contributions and active interaction with this codebase,

You're welcome, it really is my pleasure.

it's just that this feature goes against my intuitions of how the code should develop.

Then I think this PR should end here. I've ignored my gut feeling a few times in my life, and I've regretted it every time.

Concerning the scenario you want to avoid, I understand. Concerning the security guarantees, your analogy includes exactly where my view differs. Guarantees are appropriate when you pay, but I don't believe anybody's paying for this. Guarantee-wise, people just get what they pay for. That's the simple truth even if you, or we, would do (y)our utmost to fix any problem found as soon as possible, as I'd imagine.

I'll close this PR based on this conversation. If you ever do develop plans towards developing PC-BASIC that you think I could help with, please don't hesitate to let me know.