Maybe get rid of most package level variables

[nsajko]

Maybe I should have thought this through a bit more, but it seems to me that there is too much package level variables. I think that prevents memguard from being used by multiple dependencies of a Go program. For example; consider a program "package a" that depends on memguard/core and also on "package b", which in turn depends on memguard/core. Suppose both "a" and "b" try to set core.Interval, then there is a data race as it is not known which store to core.Interval happened last and which will affect the memguard funcs.

[awnumar]

That's a good point. In a situation like that one of the dependencies would unpredictably have control over the interval, and the only way to be sure of its value would be for the root module to set it itself. The only way to rectify this is likely to remove it altogether or discourage its use.

That's the only package level variable though as far as I can see.

[awnumar]

Actually thinking about this, if A relies on B and C, and both B and C rely on this package, then both B and C should have separate instances of memguard with separate interval values.

I'm not actually sure that this is the way the Go runtime handles dependencies, but intuitively it should do.

[marinewater]

both B and C should have separate instances of memguard with separate interval values.

Looks like they don't:

Interval A: 8
Interval B: 8
Interval External: 8

Interval A set to 15
Interval A: 15
Interval B: 15
Interval External: 15

Interval B set to 20
Interval A: 20
Interval B: 20
Interval External: 20

Interval External set to 25
Interval A: 25
Interval B: 25
Interval External: 25

https://github.com/marinewater/package-import-test

[awnumar]

@marinewater But it does seem as though the root preference is the preferred one: https://github.com/awnumar/package-import-test

and the only way to be sure of its value would be for the root module to set it itself

Interval A: 10
Interval B: 10
Interval External: 10

[nsajko]

The fix I had in mind is to remove var Interval and func SetInterval, and then just add an interval argument to func NewCoffer and func NewEnclave.

[awnumar]

@nsajko The interval value is used only within the core package and nowhere else. I was wary of exporting it in #100 and I'm now even more so. I don't believe that the specification of its value should be made an even more intrinsic part of the interface.

That being said, if NewCoffer took the interval as an argument and then this line in the core package hard coded the default value:

https://github.com/awnumar/memguard/blob/e3e7b95f838b7332c78c736aba627517e76b2315/core/enclave.go#L10-L13

That might work. It would no longer be a global variable. However then there would be no way for any external user to modify it.

At this point I'm in favor of adding a note of this in the documentation, raising the default slightly, and leaving it at that.

[nsajko]

I am not sure what is your understanding of the situation, but the current situation is simply unacceptably buggy - it is racy. Surely you want your packages to be safe for concurrent use by multiple dependencies of a single program.

[nsajko]

I mean you do not have to solve it as I proposed, you could instead have a method to modify the interval, the important thing is that it must not be a package level variable.

[awnumar]

1. A method would suffer from exactly the same issue as there is only one global key.
2. Operations are atomic so it's not "unsafe" racy. The issue is unpredictability in the precise value when multiple dependencies want to specify it. I see this as fine as the value does not matter too much as long as it is low enough.
3. buffers is protected by a mutex.

The only other solution I am happy with is to remove the option to set the value altogether.

[nsajko]

Really the underlying issue here is that you are using package level variables at all. That is only OK to do in general if they are effectively constant (only assigned to during initialization).

[awnumar]

It is not in func NewBuffer

https://github.com/awnumar/memguard/blob/e3e7b95f838b7332c78c736aba627517e76b2315/core/buffer.go#L96-L97

https://github.com/awnumar/memguard/blob/e3e7b95f838b7332c78c736aba627517e76b2315/core/buffer.go#L222-L228

[nsajko]

OK, sorry, it is protected by a mutex. But you would not need mutexes if the variables were not package level.

[awnumar]

There's no way around having to keep global state in an application like this. I'm sorry it makes you uncomfortable but "package-level variables" (i.e. globals) are not inherently bad.

The issue with SetInterval I can at least sympathize with since callers are able to mutate it, but the rest of these are not even exported.

[nsajko]

There's no way around having to keep global state in an application like this.

Not true.

The issue with SetInterval I can at least sympathize with since callers are able to mutate it, but the rest of these are not even exported.

The callers are able to mutate the other ones too, through the exported funcs, that is why you need locking (with mutual exclusion locks). You would not need locking if the variables were session based instead of package level.

[nsajko]

https://dave.cheney.net/2017/06/11/go-without-package-scoped-variables

[awnumar]

There's no way around having to keep global state in an application like this.

Not true.

Thinking about this more deeply, I'm still not convinced that it's possible to remove globals from the library. Let me explain.

In a session-based scheme where the caller creates a session that has a local key and list of buffers, there would still need to be a global buffer containing the list of sessions in order to implement the safe termination procedures. Perhaps it's still worth the effort in order to get rid of having to handle init() functions being called multiple times.

You would not need locking if the variables were session based instead of package level.

This is simply not true. Each session could also have multiple threads accessing the session state and this would need to be internally thread-safe.

For now, the pull request I just opened (#114) gets rid of the specification of the interval value among some other small changes. I will continue looking into the feasibility of a session-based approach and how specifically it would be implemented.