Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Coroutines doesn't play well with JNLP security model #216

Closed
NikolayMetchev opened this issue Feb 3, 2018 · 46 comments
Closed

Coroutines doesn't play well with JNLP security model #216

NikolayMetchev opened this issue Feb 3, 2018 · 46 comments
Labels

Comments

@NikolayMetchev
Copy link
Contributor

NikolayMetchev commented Feb 3, 2018

jnlptest.zip
I have created and attached a simple swing app that uses JNLP and co-routines.
I have in this app code that uses both swingworker and co-routines to download the html from google.com.
It also does this using java.net.HttpURLConnection and apache httpclient.

The swingworker code when run through JNLP works just fine without any problems (as long as the jnlp is signed).

The co-routines code exhibits 2 different behaviours:

  1. java.net.HttpURLConnection - results in warning dialogs poping up asking the user for permissions to connect to google
  2. apache http: java.security.AccessControlException

Exception in thread "AWT-EventQueue-2" java.util.concurrent.CompletionException: java.security.AccessControlException: access denied ("java.util.PropertyPermission" "java.version" "read")
at java.util.concurrent.CompletableFuture.encodeThrowable(Unknown Source)
at java.util.concurrent.CompletableFuture.completeThrowable(Unknown Source)
at java.util.concurrent.CompletableFuture$AsyncSupply.run(Unknown Source)
at java.util.concurrent.CompletableFuture$AsyncSupply.exec(Unknown Source)
at java.util.concurrent.ForkJoinTask.doExec(Unknown Source)
at java.util.concurrent.ForkJoinPool$WorkQueue.runTask(Unknown Source)
at java.util.concurrent.ForkJoinPool.runWorker(Unknown Source)
at java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source)
Caused by: java.security.AccessControlException: access denied ("java.util.PropertyPermission" "java.version" "read")
at java.security.AccessControlContext.checkPermission(Unknown Source)
at java.security.AccessController.checkPermission(Unknown Source)
at java.lang.SecurityManager.checkPermission(Unknown Source)
at com.sun.javaws.security.JavaWebStartSecurity.checkPermission(Unknown Source)
at java.lang.SecurityManager.checkPropertyAccess(Unknown Source)
at java.lang.System.getProperty(Unknown Source)
at org.apache.http.util.VersionInfo.getUserAgent(VersionInfo.java:321)
at org.apache.http.impl.client.HttpClientBuilder.build(HttpClientBuilder.java:1046)
at org.apache.http.impl.client.HttpClients.createDefault(HttpClients.java:56)
at jnlp.MainKt.getHTMLWithApache(main.kt:88)
at jnlp.MainKt$main$4.invoke(main.kt:29)
at jnlp.MainKt$main$4.invoke(main.kt)
at jnlp.MainKt$coroutine$1$1$1.get(main.kt:38)
at jnlp.MainKt$coroutine$1$1$1.get(main.kt)
... 6 more

To reproduce you should be able to just unzip the directory and double click on build/jnlp/launch.jnlp

@elizarov
Copy link
Contributor

elizarov commented Feb 8, 2018

Are you sure it is a problem with coroutines? I'm not seeing any kotlinx.coroutines code in your stack trace. I'm seeing a problem Apache HTTP. It looks like with JNLP you should sign all the libraries you are using with your own certificate and grant them all the necessarily permissions.

@elizarov elizarov changed the title Co-routines doesn't play well with JNLP security model Coroutines doesn't play well with JNLP security model Feb 8, 2018
@NikolayMetchev
Copy link
Contributor Author

Hello @elizarov
Unfortunately Java truncated the stack trace for some reason. At the bottom of the stack trace you have
"... 6 more" in there it should have kotlinx.coroutines classes.
All libraries are signed with a certificate. This is done using gradle and the "de.gliderpilot.jnlp" plugin.
You should find everything you need in the zip file.

@userquin
Copy link

userquin commented Feb 9, 2018

hi:

Have yo try to execute http call (httpurlconnection) with AccessController.doProvileged???

Even if the JAR is signed, you must use AccessController.doPrivileged, JNLP or applet or in a sandbox with security manager enabled.

the link for AccessController

@NikolayMetchev
Copy link
Contributor Author

@userquin I am aware of AccessController but I don't think that is a viable solution. The JNLP in question is signed and has the following tag which means it shouldn't need to request permissions:

<security>
    <all-permissions />
  </security> 

The SwingWorker version and coroutine version executes exactly the same bit of code, the difference being that coroutine code behaves differently.

@userquin
Copy link

try use this:

fun String.fetch(): String = AccessController.doPrivileged(PrivilegedAction<String> {
    URL(this).let {
        var httpURLConnection: HttpURLConnection? = null
        try {
            httpURLConnection = it.openConnection() as HttpURLConnection
            httpURLConnection.requestMethod = "GET"
            BufferedReader(InputStreamReader(httpURLConnection.inputStream)).use {
                it.readLines().joinToString("")
            }
        } catch (e: Exception) {
            throw RuntimeException(e)
        } finally {
            httpURLConnection?.disconnect()
        }
    }
})

@NikolayMetchev
Copy link
Contributor Author

@userquin I know that will work, however I shouldn't have to do that. In our more complicated production app I had to add these calls all over the place where coroutines were used and in the end the application still didn't work because it ended up with a NullPointerException. I am looking for an explanation why in the example app that I have attached as part of this bug report the SwingWorker code works as expected and the coroutines code doesn't.

@userquin
Copy link

ok, the problem may be the pool you are using to execute the coroutine, maybe providing your own context using on it a security manager... I'll trying to find the code I use...

@userquin
Copy link

I have downloaded your zip and I have executed the jnlp from build\jnlp directory (enabling file:// in java control panel: Java 1.8) and HttpURLConnection works, Apache not working. The only difference from swing worker and coroutine is java web start prompts a confirm dialog to connect to google using coroutine (confirm in spanish):

app

coroutine

coroutine-result

@NikolayMetchev
Copy link
Contributor Author

@userquin thanks for testing. You have just confirmed everything I have said in the original bug report. If you enable the JNLP console you will see the exception that happens with the apache version there.

@NikolayMetchev
Copy link
Contributor Author

Also I should point out that you can open the project in Idea and view/edit the code to try and see if you can figure out why coroutines is breaking the JNLP security model.

@userquin
Copy link

As I mention before, the problem is the pool that Swing (launch(Swing)) context uses: it must delegate on security manage or at least check if one exists and then enqueue the pool in the same group the security manager runs.

The problem with its pool is that in its context there is no way to elevate required permissions, so you must create one that checks if a security manager.

One simple way is provide to your pool a ThreadFactory; then check if a security manager exists, if so, then create threads using the same group as the security manager and remeber to create the thread with a runnable that switchs the context class loader to the loader that creates de access controller context required to elevate the privileges.

In 5 minutes I provide an example...

@userquin
Copy link

Executors.privilegedThreadFactory method can be used to create a new FixedThreadPool (also from Executors class) providing number of threads and privilegedThreadFactory as threadfactory.

By default launch uses CommonPool:

public val DefaultDispatcher: CoroutineDispatcher = CommonPool
...
public fun launch(
    context: CoroutineContext = DefaultDispatcher,

For example, kotlinx.coroutines.experimental.CommonPool. uses this:

    private fun createPool(): ExecutorService {
        val fjpClass = Try { Class.forName("java.util.concurrent.ForkJoinPool") }
            ?: return createPlainPool()
        if (!usePrivatePool) {
            Try { fjpClass.getMethod("commonPool")?.invoke(null) as? ExecutorService }
                ?.let { return it }
        }
        Try { fjpClass.getConstructor(Int::class.java).newInstance(defaultParallelism()) as? ExecutorService }
            ?. let { return it }
        return createPlainPool()
    }

    private fun createPool(): ExecutorService {
        val threadId = AtomicInteger()
        return Executors.newFixedThreadPool(defaultParallelism()) {
            Thread(it, "CommonPool-worker-${threadId.incrementAndGet()}").apply { isDaemon = true }
        }
    }

Create a new CoroutineDispatcher like CommonPool and just change the way it creates the pool:

    private fun createPool(): ExecutorService {
        val threadId = AtomicInteger()
        return Executors.newFixedThreadPool(defaultParallelism(), Executors.privilegedThreadFactory()) {
            Thread(it, "PrivilegedCommonPool-worker-${threadId.incrementAndGet()}").apply { isDaemon = true }
        }
    }

and finally just call launch with the new one CoroutineDispatcher created (copy/paste CommonPool and change only createPool and remove createPlainPool: you will use this on desktop environment, so using fork join does not matter):

object PrivilegedCommonPool: CoroutineDispatcher() {
    private fun createPool(): ExecutorService {
        val threadId = AtomicInteger()
        return Executors.newFixedThreadPool(defaultParallelism(), Executors.privilegedThreadFactory()) {
            Thread(it, "PrivilegedCommonPool-worker-${threadId.incrementAndGet()}").apply { isDaemon = true }
        }
    }
}

define your dispatcher and your privileged dispatcher:

public val PrivilegedDispatcher: CoroutineDispatcher = PrivilegedCommonPool
...
public fun launchPrivileged(
    context: CoroutineContext = PrivilegedDispatcher,
    start: CoroutineStart = CoroutineStart.DEFAULT,
    block: suspend CoroutineScope.() -> Unit
): Job {
    val newContext = newCoroutineContext(context)
    val coroutine = if (start.isLazy)
        LazyStandaloneCoroutine(newContext, block) else
        StandaloneCoroutine(newContext, active = true)
    coroutine.initParentJob(context[Job])
    start(block, coroutine, coroutine)
    return coroutine
}

and use it:

launchPrivileged({
      showResult(jFrame, CompletableFuture.supplyAsync {
        getHtml()
      }.await())
})

or you just name it launch and import your launch instead kotlin, the code will be the same, just change 1 or 2 imports in the file...

@userquin
Copy link

ok, cannot be done, we must use DefaultExecutor it is internal... We must request this new feature to kotlin:

@elizarov can this be included in kotlin coroutines (or in swing or jnlp extension)?

Include in CoroutineContext.kt

/**
 * This is the privileged [CoroutineDispatcher] that is used by all standard builders like
 * [launch], [async], etc if no dispatcher nor any other [ContinuationInterceptor] is specified in their context.
 *
 * It is currently equal to [PrivilegedCommonPool], but the value is subject to change in the future.
 */
public val PrivilegedDispatcher: CoroutineDispatcher = PrivilegedCommonPool

New PrivilegedCommonPool.kt

package kotlinx.coroutines.experimental

import java.util.concurrent.*
import kotlin.coroutines.experimental.CoroutineContext

object PrivilegedCommonPool: CoroutineDispatcher() {

    @Volatile
    private var _pool: Executor? = null

    private fun createPool(): ExecutorService {
        return Executors.newFixedThreadPool(defaultParallelism(), Executors.privilegedThreadFactory())
    }

    private fun defaultParallelism() = (Runtime.getRuntime().availableProcessors() - 1).coerceAtLeast(1)

    @Synchronized
    private fun getOrCreatePoolSync(): Executor =
            _pool
                    ?: createPool().also { _pool = it }

    override fun dispatch(context: CoroutineContext, block: Runnable) =
            try { (PrivilegedCommonPool._pool ?: PrivilegedCommonPool.getOrCreatePoolSync()).execute(timeSource.trackTask(block)) }
            catch (e: RejectedExecutionException) {
                timeSource.unTrackTask()
                DefaultExecutor.execute(block)
            }

    // used for tests
    @Synchronized
    internal fun usePrivatePool() {
        shutdown(0)
        PrivilegedCommonPool._pool = null
    }

    // used for tests
    @Synchronized
    internal fun shutdown(timeout: Long) {
        (PrivilegedCommonPool._pool as? ExecutorService)?.apply {
            shutdown()
            if (timeout > 0)
                awaitTermination(timeout, TimeUnit.MILLISECONDS)
            shutdownNow().forEach { DefaultExecutor.execute(it) }
        }
        PrivilegedCommonPool._pool = Executor { throw RejectedExecutionException("PrivilegedCommonPoolwas shutdown") }
    }

    // used for tests
    @Synchronized
    internal fun restore() {
        shutdown(0)
        PrivilegedCommonPool._pool = null
    }

    override fun toString(): String = "PrivilegedCommonPool"
}

with these changes we can use launch(PrivilegedCommonPool) {...} when running on a sandbox (SecurityManager) solving the problem.

by the way, how can I test this on intellij?

@elizarov
Copy link
Contributor

elizarov commented Feb 10, 2018

I have a few questions here.

  1. Do we really need a separate PriveledgedCommonPool? What if we just change the code for the DefaultDispatcher so that when SecurityManager is set, then it applies the corresponding "fixes"?

  2. Is privilegedThreadFactoryreally needed? What puzzles me here is that it works via SwingWorker that does not use privilegedThreadFactory. The only clue I was I able to find in the source code of the SwingWorker is that it uses DefaultThreadFactory to create its thread pool and it has the following code:

SecurityManager s = System.getSecurityManager();
group = (s != null) ? s.getThreadGroup() : Thread.currentThread().getThreadGroup();

So, when SecurityManager is set, it creates thread in security manager's thread group. Would adding a similar code to CommonPool help?

@NikolayMetchev
Copy link
Contributor Author

NikolayMetchev commented Feb 10, 2018

Hello, It seems I am beginning to understand what is going on here. Kotlin Coroutines are using the ForkJoinPool and it seems that this pool creates threads that have no privileges. I found a very nice blog post which talks about this:
http://blog.aggregat4.net/posts/2015-04-14-caution-advised-when-using-the-java-forkjoinpool-or-parallel-streams-with-a-securityManager.html

I tried modifying the example code to use the ForkJoinPool directly and the bug reproduced. Unfortunately according to the article there doesn't appear to be a simple workaround.

My suggestion right now is to allow some way of specifying a different threadPool other than the ForkJoinPool because as far as I can tell that thread pool cannot be configured to create threads with the correct security within JNLP.

@userquin
Copy link

userquin commented Feb 10, 2018

@NikolayMetchev but why HttpURLConnection is working? it uses the same thread pool Apache HttpClient sample uses. You must call HttpClient code with AccessController.doPrivileged and should work (see 2 below).

@elizarov

  1. there is no need if the CommonPool fix it
  2. SwingWorker only works with HttpUrlConnection not with Apache HttpClient: it is because HttpURLConnection just do all calls with AccessController.doPrivileged instead direct call to connect, resolve, etc.. JNLP security manager stack will prompt the required permissions to be granted to executed the privileged code in a explicit way (that is, show a dialog requesting those permissions: ala android request permissions from M and later). In that case use privilegedThreadFactory or not does not matter, just because jws takes care to request required permissions.

And yes, it would help if CommonPool uses this aproach if security manager is present

@NikolayMetchev
Copy link
Contributor Author

@userquin SwingWorker works with both HttpURLConnection and apache HttpClient and there is no need for AccessController. As I said in my earlier comment the problem is the ForkJoinPool that cannot be used in JNLP if you wish to perform any privileged actions.

@userquin
Copy link

HttpURLConnection just works if you grant the permission from the dialog, if you revoke it you just get the original exception (the same stack but with different context). The problem is not the common pool, is the way HttpClient is done (see how sun.net.www.protocol.http.HttpURLConnection uses AccessController.doPrivileged all the time). By the way, I suggest you to use always this method when doing privileged operations in a sandbox ;):

getHTML = ForkJoinPool.commonPool-worker-1
Exception in thread "AWT-EventQueue-2" java.util.concurrent.CompletionException: java.security.AccessControlException: access denied ("java.net.SocketPermission" "www.google.com:80" "connect,resolve")
	at java.util.concurrent.CompletableFuture.encodeThrowable(Unknown Source)
	at java.util.concurrent.CompletableFuture.completeThrowable(Unknown Source)
	at java.util.concurrent.CompletableFuture$AsyncSupply.run(Unknown Source)
	at java.util.concurrent.CompletableFuture$AsyncSupply.exec(Unknown Source)
	at java.util.concurrent.ForkJoinTask.doExec(Unknown Source)
	at java.util.concurrent.ForkJoinPool$WorkQueue.runTask(Unknown Source)
	at java.util.concurrent.ForkJoinPool.runWorker(Unknown Source)
	at java.util.concurrent.ForkJoinWorkerThread.run(Unknown Source)
Caused by: java.security.AccessControlException: access denied ("java.net.SocketPermission" "www.google.com:80" "connect,resolve")
	at java.security.AccessControlContext.checkPermission(Unknown Source)
	at java.security.AccessController.checkPermission(Unknown Source)
	at java.lang.SecurityManager.checkPermission(Unknown Source)
	at com.sun.javaws.security.JavaWebStartSecurity.checkPermission(Unknown Source)
	at java.lang.SecurityManager.checkConnect(Unknown Source)
	at com.sun.javaws.security.JavaWebStartSecurity.checkConnectHelper(Unknown Source)
	at com.sun.javaws.security.JavaWebStartSecurity.checkConnect(Unknown Source)
	at sun.net.www.http.HttpClient.openServer(Unknown Source)
	at sun.net.www.http.HttpClient.<init>(Unknown Source)
	at sun.net.www.http.HttpClient.New(Unknown Source)
	at sun.net.www.http.HttpClient.New(Unknown Source)
	at sun.net.www.protocol.http.HttpURLConnection.getNewHttpClient(Unknown Source)
	at sun.net.www.protocol.http.HttpURLConnection.plainConnect0(Unknown Source)
	at sun.net.www.protocol.http.HttpURLConnection.plainConnect(Unknown Source)
	at sun.net.www.protocol.http.HttpURLConnection.connect(Unknown Source)
	at sun.net.www.protocol.http.HttpURLConnection.getInputStream0(Unknown Source)
	at sun.net.www.protocol.http.HttpURLConnection.getInputStream(Unknown Source)
	at jnlp.MainKt.getHTML(main.kt:78)
	at jnlp.MainKt$main$3.invoke(main.kt:28)
	at jnlp.MainKt$main$3.invoke(main.kt)
	at jnlp.MainKt$coroutine$1$1$1.get(main.kt:38)
	at jnlp.MainKt$coroutine$1$1$1.get(main.kt)
	... 6 more

By the way, can you test if this works?:

fun getHTMLWithApache(): String = AccessController.doPrivileged(PrivilegedAction<String> {
    val connection = HttpClients.createDefault()
    val execute = connection.execute(HttpGet("http://www.google.com"))
    EntityUtils.toString(execute.entity)
})

@userquin
Copy link

userquin commented Feb 10, 2018

I'm talking about corourines not swingworker. My previous respnse to @elizarov was wrong, it is not working with the forkjoin problem you mention.

  1. is about coroutine not related to SwingWorker...

@userquin
Copy link

By the way, in the previous stack trace you can see how the java web start stack request the permissions, but this only occurs if you use AccessController.doPrivileged call. Using HttpURLConnection, java do it for you but Apache HttpClient no.

@NikolayMetchev
Copy link
Contributor Author

@userquin I wrote the sample app to demonstrate the problem. In the general case an app can choose to do all sorts of privileged actions in a coroutine and it may invoke 3rd party library code for which it may not necessarily be possible to correct the security context, by wrapping in an AccessController.doPriviledged().

In order to solve this in the general case I think we need to be able to override the threadPool used in co-routines (or at the very least the pool used in the Swing coroutines context).

@userquin
Copy link

OK, that is when privilegedThreadFactory come into place!!!

  1. create the pool with privilegedThreadFactory as ThreadFactory (or make your own): this will elevate calling 3rd party code into your security stack "context" (see java.security.AccessController.getContext())
  2. You must ALWAYS call privileged potentially 3rd party code with AccessController.doPrivileged

@NikolayMetchev
Copy link
Contributor Author

@userquin are you sure your approach would work in the example app? The reason for using the Swing CoroutineDispatcher is that we want the updates to the GUI to be done on the AWT event thread.
launch(PrivilegedCommonPool) {...} will not have such guarantees. Ideally we should still be able to use the Swing CoroutineDispatcher.

@userquin
Copy link

userquin commented Feb 10, 2018

As a summary, it is not a problem with how CommonPool works, it only needs to fix the use of the threadgroup's security manager if one is present.

If anyone tries to use a 3rd party library that does not check privileged operations via AccessController.doPrivileged calls, security manager cannot be bypassed, so depending on the context, things will work or not. In the case of coroutines we must take care with it, just because it will execute the code, so the context is different that using SwingWorker.

In this scenario, you are trying to execute unprivileged code from 3rd party library in a way (coroutine) java does not allow you. With HttpURLConnection works just because jws request the permissions and because HttpURLConnection uses AccessController.doPrivileged.

In the other hand, why is working both examples using SwingWorker? Just because SwingWorker takes care and create its own pool in the same secutity manager thread group.

@userquin
Copy link

userquin commented Feb 10, 2018

yes, it should work, wait response from @elizarov

In your code you are using CommonPool not Swing:

private fun coroutine(jFrame: JFrame, getHtml: () -> String): (Any) -> Unit {
  return {
    launch({
      showResult(jFrame, CompletableFuture.supplyAsync {
        getHtml()
      }.await())
    })
  }
}

@userquin
Copy link

upps, sorry, your launch is using Swing....

I'm going to review your code... With intellij 2017.1.5 cannot configure your projec., some gradle problem with proxy and url...

@NikolayMetchev
Copy link
Contributor Author

Ahh, sorry I'm using the EAP

@userquin
Copy link

it should work, Swing just declares the continuation:

from Builders.launch documentation:

If the context does not have any dispatcher nor any other [ContinuationInterceptor], then [DefaultDispatcher] is used.

@NikolayMetchev
Copy link
Contributor Author

Not sure I follow.
if I use launch(PrivilegedCommonPool) {...} then I wont be dispatching events to the GUI thread. If I use launch(Swing) {...} then I will be using CommonPool and hence the ForkJoinPool. Is there a way to combine the two?

@userquin
Copy link

No, you just use launch(Swing) {}, kotlin must change CommonPool to configure the thread group from the security manager and you must always wrap your code with AccessController.doPrivileged.

@userquin
Copy link

when launch create the coruotine using Swing as parameter, it would use internally CommonPool with ForkJoin executor... then the continuation will be invokeg throw Swing, that is EDT.

@NikolayMetchev
Copy link
Contributor Author

It doesn't look like it's possible to change the thread group if you use the ForkJoinPool unless you do some hack. Not sure how you plan on changing CommonPool.

@elizarov
Copy link
Contributor

We can change implementation of the CommonPool, so that if SecurityManager is set it does not use ForkJoinPool, but creates its own thread-pool using privileged thread factory. That should help, I think.

@elizarov elizarov added bug and removed question labels Feb 10, 2018
@userquin
Copy link

I think it would be a better solution to allow enable it, via some hack in the dispatcher (for example a flag). In fact, it would be a better solution to include 2 flags, one to enable it and other to enable privileged thread factory (similar solution to increase the size of the pool).

@elizarov
Copy link
Contributor

@userquin It don't think it is going to help in JNLP case, since with JNLP it is also notoriously difficult to pass any flags/system properties. I don't see how it is going to harm either. Who in the world uses SecurityManager but for JNLP?

@userquin
Copy link

OK, I agree with you, also it is the simple way to workaround the problem...

For your info:
In the server side I always use SecurityManager because I have to deploy disparate applications coming from external clients that change jndi base names, make JVM shutdowns, access files in the machine, etc ... and I dont want to allow do these operations, so I must restrict the access to some resources in the JVM ecosystem.

@NikolayMetchev
Copy link
Contributor Author

Hello,
I've had a 2nd look and I have discovered that changing CommonPool will not fix the example code I have supplied. The problem with the example code is that in order to create the background thread I invoke CompletableFuture.supplyAsync(Supplier<U> supplier). This is actually the method that is using the ForkJoinPool. In order to fix the code I should be invoking CompletableFuture.supplyAsync(Supplier<U> supplier, Executor executor) and make sure I don't pass a ForkJoinPool as the 2nd argument.

However if I use launch(CommonPool) {...} within a JNLP context I will hit the same problem and indeed I amended my example code to prove this fact.

I also then built my sample app with a modified CommonPool.kt that checks if a SecurityManager exists and create a privileged pool. This did indeed fix the problem.

Would you like me to create a pull request with my suggested changes?

@NikolayMetchev
Copy link
Contributor Author

I've created a pull request:
#237

@fvasco
Copy link
Contributor

fvasco commented Feb 11, 2018

The metod Executors.privilegedThreadFactory() works only in some cases (see documentation), so having a CommonPool with undefined behavior may not be a nice to have.
Moreover a shared Privileged Executor is a easy way to disable all SecurityManager's checks, this may be an issue for some applications.

In my -humble- opinion a globally shared Privileged Executor is a big issue on JVM: you can call a privileged method only if you is albe to execute a privileged action.

@NikolayMetchev
Copy link
Contributor Author

@fvasco Thanks for your comment. I wasn't too sure about a privileged thread factory either. For my example code to work the required change was just not to use ForkJoinPool. So if we change CommonPool to use createPlainPool() if there is a SecurityManager present then in my particular case everything should be fine. Would you like me to amend my pull request?

@userquin
Copy link

I also agree with @fvasco to NOT use a privileged thread factory, you MUST call always AccessController.doPrivileged to elevate privileges and only change CommonPool to create the executor in the same security manager's thread group if one exists.

In your PR you must use Executors.defaultThreadFactory() instead Executors.privilegedThreadFactory()

@NikolayMetchev
Copy link
Contributor Author

I've amended the pull request to use createPlainPool().

@fvasco
Copy link
Contributor

fvasco commented Feb 11, 2018

Is it the following function enough to fix this issue?

fun <T> AccessControlContext.runPrivileged(block: () -> T): T = 
        AccessController.doPrivileged(PrivilegedAction { block() }, this)

I suppose there is a better way than execute always all code as privileged.

Edit: this should work with Apache HTTP

@userquin
Copy link

@fvasco when using 3rd party libs you must ALWAYS call the code as privileged (Java Web Start or Applet security manager). If you invoke the unprivileged code from a context where it is not allowed then you will have problems.

In the context of @NikolayMetchev, I think (I must check it) there is no need to change CommonPool, and a call to apache http client code with doPrivileged must work, even with ForkJoin executor.

If in the previous context, ForkJoin still fails, changing CommonPool to switch from ForkJoin to:

Executors.newFixedThreadPool(defaultParallelism(), Executors.defaultThreadFactory())

when a SecurityManager is configured, calling apache http client code with doPrivileged should work.

@fvasco
Copy link
Contributor

fvasco commented Feb 14, 2018

Hi @userquin,
if I understand correctly this thread then the issue is how to invoke privileged code inside a coroutine, regardless the dispatcher.

Unfortunately I don't have a toy project for your use case, however my proposal is to save the Security Context inside your application (withot sharing it) and to use it if and only if it is required (possibly without avoidable thread switch).

I dump to you some code to sketch my idea.

internal lateinit var appSecurityContext: AccessControlContext

fun <T> AccessControlContext.runPrivileged(block: () -> T): T =
        AccessController.doPrivileged(PrivilegedAction { block() }, this)

fun <T> privileged(block: () -> T): T = appSecurityContext.runPrivileged(block)

fun main(vararg args: String) {
    appSecurityContext = AccessController.getContext()
    launch {
        privileged {
            // god mode
        }
    }
}

@userquin
Copy link

Hi @fvasco

Not exactly 100% but almost: the problem is use Swing as the continuation, with CommonPool as Disptacher (replacing javax.swing.SwingWorker). Your code is not exactly what originally requested:

launch(Swing) {
  [optionally try]
   async task in common pool executor
   continuation con EDT via Swing
  [optionally catch]
      continuation on EDT via Swing
}

Using Android example:

async(UI) {
  try {
     val response: Deferred<String> = bg {  doWork() } // Common Pool
     uiView.text = response.await() // continuation on UI thread 
  } catch(e: Exception) {
      uiView.text = e.message // continuation on UI thread 
  }
}

The rest of the history remains...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants