Run evaluation of inlined quotes in secured sandbox #4111
Conversation
nicolasstucki
force-pushed
the
add-splicer-security-manager
branch
10 times, most recently
from
51e4871 to
7146996
Compare
| @@ -38,15 +38,18 @@ object Sandbox { | |||
|
|
|||
| object SandboxSecurityManager { | |||
| @volatile private[this] var running: Int = 0 | |||
| private def getRunning = running | |||
| private def incRunning() = running += 1 | |||
| private def decRunning() = running -= 1 | |||
There was a problem hiding this comment.
@nicolasstucki FWIW: if the object is shared and you want incRunning to be atomic, you do need synchronized, volatile is powerless to keep together the read and the write as a single atomic action.
(No I don't know if those conditions hold, hence FWIW, sorry for the noise)
There was a problem hiding this comment.
I knlow, I stumbled with some other bug and I'm just trying to move things too see what fixes it.
There was a problem hiding this comment.
BTW, the atomicity was given by the synchronized block in the only method that could access incRunning.
nicolasstucki
force-pushed
the
add-splicer-security-manager
branch
3 times, most recently
from
2c8b850 to
f3e3aa5
Compare
nicolasstucki
force-pushed
the
add-splicer-security-manager
branch
from
f3e3aa5 to
6fc1527
Compare
| thread.start() | ||
| thread.join(timeout) | ||
| if (thread.isAlive) { | ||
| // TODO kill the thread? |
There was a problem hiding this comment.
Any chance to add a test with an infinite loop to test this case?
Because Dotty can be embedded in build tools VMs, this means we must kill the thread I think.
I'd use thread.interrupt() first, and specify/hope that the thread must not catch InterruptedException. If the thread doesn't handle interrupt by exiting, I'm not sure what's a sensible behavior — using stop might leave monitors held hence cause a deadlock release monitors while data is inconsistent causing corruption, not using stop might leave a thread wasting CPU, so I suspect the thread should be stoped and the compiler should not be reused.
There was a problem hiding this comment.
Shared data that might be corrupted by Thread.stop includes
- shared static mutable data such as the name table (Slow memory leak in Dotty's parser #1584), that is reused across all compiler instances in the same classloader. More such fields might exist.
- any compiler-specific data that is internally mutable and shared across threads. The second isn't a problem currently because compiler instances are currently not reused by build tools, though that might change in the future.
After some discussion on Gitter and with Allan, I learned that build tools usually fork Scala compilers in the same JVM (though that's configurable) — they typically create fresh compiler instances (which is safer) though @smarter suggests it could be faster to reuse them sometimes (which is only safe for instances which didn't get corrupted).
|
test performance please |
|
performance test scheduled: 3 job(s) in queue, 1 running. |
|
performance test failed: Please check http://lamppc37.epfl.ch:8000/pull-4111-04-11-16.21.out for more information |
Need to be rebased |
|
|
||
| private def isClassLoading: Boolean = { | ||
| try { | ||
| enabledFlag.set(false) // Disable security do the check |
There was a problem hiding this comment.
*to do the check
| permission match { | ||
| case _: ReflectPermission => | ||
| // allow reflection, needed for interpreter | ||
| // TODO restrict more? |
There was a problem hiding this comment.
Looking at the doc of ReflectionPremission it looks like we could disallow suppressAccessChecks to guard against access to private fields.
There was a problem hiding this comment.
Now that this code is not interpreted we can disallow all ReflectPermission
| object Sandbox { | ||
|
|
||
| /** Timeout in milliseconds */ | ||
| final val timeout = 3000 // TODO add a flag to allow custom timeouts |
There was a problem hiding this comment.
Maybe this is worth addressing before merging?
|
@nicolasstucki Why is this PR |
nicolasstucki
force-pushed
the
add-splicer-security-manager
branch
from
6fc1527 to
57b06d3
Compare
|
test performance with #quotes please |
|
performance test scheduled: 3 job(s) in queue, 1 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/4111/ to see the changes. Benchmarks is based on merging with master (9c83d27) |
nicolasstucki
force-pushed
the
add-splicer-security-manager
branch
from
e18c977 to
79ae75e
Compare
No description provided.