Skip to content

Commit

Permalink
twitter-server: Add SecurityManager check to ContentionHandler
Browse files Browse the repository at this point in the history
Problem

Attempting to access the `ManagementFactory.getThreadMXBean` to enable
contention monitoring can throw a `SecurityException` if the `SecurityManager`
does not allow for that permission.

Solution

Ensure that the `ContentionHandler` checks the `SecurityManager` before
attempting to instantiate a `ContentionSnapshot`.

JIRA Issues: CSL-10056

Differential Revision: https://phabricator.twitter.biz/D531873
  • Loading branch information
cacoco authored and jenkins committed Aug 12, 2020
1 parent 61df084 commit ce783a3
Show file tree
Hide file tree
Showing 3 changed files with 118 additions and 14 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ Note that ``PHAB_ID=#`` and ``RB_ID=#`` correspond to associated messages in com
Unreleased
----------

* Check SecurityManager permissions in the `ContentHandler` to ensure that contention
snapshotting is allowed. ``PHAB_ID=D531873``

20.8.0
------

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,52 @@ import com.twitter.finagle.http.{Request, Response}
import com.twitter.jvm.ContentionSnapshot
import com.twitter.server.util.HttpUtils.newOk
import com.twitter.util.Future
import com.twitter.util.logging.Logging
import java.lang.management.ManagementPermission

class ContentionHandler extends Service[Request, Response] with Logging {

// Must check the installed java.lang.SecurityManager for java.lang.management.ManagementPermission("control")
// access. If no java.lang.SecurityManager, we assume the instantiation of the ContentionSnapshot will succeed.
private[this] val contentionSnapshot: Option[ContentionSnapshot] = {
val permission = new ManagementPermission("control")
Option(System.getSecurityManager) match {
case Some(securityManager) =>
try {
securityManager.checkPermission(permission)
Some(new ContentionSnapshot)
} catch {
case e: SecurityException => // not allowed
warn("Contention snapshotting is not allowed by SecurityManager.", e)
None
}
case _ => // no security manager
Some(new ContentionSnapshot)
}
}

class ContentionHandler extends Service[Request, Response] {
private[this] val snapshotter = new ContentionSnapshot
def apply(req: Request): Future[Response] = {
val snap = snapshotter.snap
val deadlockMsg =
if (snap.deadlocks.isEmpty) ""
else {
"DEADLOCKS:\n\n%s\n\n".format(snap.deadlocks.mkString("\n\n"))
}
contentionSnapshot match {
case Some(snapshot) =>
val snap = snapshot.snap()
val deadlockMsg =
if (snap.deadlocks.isEmpty) ""
else {
"DEADLOCKS:\n\n%s\n\n".format(snap.deadlocks.mkString("\n\n"))
}

val msg = "%sBlocked:\n%s\n\nLock Owners:\n%s".format(
deadlockMsg,
snap.blockedThreads.mkString("\n"),
snap.lockOwners.mkString("\n")
)
val msg = "%sBlocked:\n%s\n\nLock Owners:\n%s".format(
deadlockMsg,
snap.blockedThreads.mkString("\n"),
snap.lockOwners.mkString("\n")
)

newOk(msg)
newOk(msg)
case _ =>
val msg =
"Contention snapshotting is not enabled due to SecurityManager restrictions.\n" +
"Please ensure that the java.lang.management.ManagementPermission(\"control\") is allowed."
newOk(msg)
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
package com.twitter.server.handler

import com.twitter.conversions.DurationOps._
import com.twitter.finagle.http.{Request, Status}
import com.twitter.util.{Await, Awaitable, Promise}
import java.util.concurrent.locks.ReentrantLock
import org.scalatest.concurrent.Eventually
import org.scalatest.funsuite.AnyFunSuite
import org.scalatest.time.{Millis, Seconds, Span}

object ContentionHandlerTest {
class Philosopher {
val ready = new Promise[Unit]
private val lock = new ReentrantLock()
def dine(neighbor: Philosopher): Unit = {
lock.lockInterruptibly()
ready.setValue(())
Await.ready(neighbor.ready)
neighbor.dine(this)
lock.unlock()
}
}
}

class ContentionHandlerTest extends AnyFunSuite with Eventually {
import ContentionHandlerTest._

implicit override val patienceConfig: PatienceConfig = {
PatienceConfig(timeout = scaled(Span(15, Seconds)), interval = scaled(Span(5, Millis)))
}

test("ContentionHandler#instantiation") {
new ContentionHandler
}

test("ContentionHandler#detect deadlocks") {
val handler = new ContentionHandler

val descartes = new Philosopher()
val plato = new Philosopher()

val d = new Thread(new Runnable() {
def run(): Unit = { descartes.dine(plato) }
})
d.start()

val p = new Thread(new Runnable() {
def run(): Unit = { plato.dine(descartes) }
})
p.start()
Await.all(descartes.ready, plato.ready)

eventually {
assertResponseMessage(handler, "DEADLOCKS:")
}
d.interrupt()
p.interrupt()
p.join()
d.join()
assertResponseMessage(handler, "")
}

private[this] def await[T](a: Awaitable[T]): T = Await.result(a, 2.seconds)

private[this] def assertResponseMessage(handler: ContentionHandler, message: String): Unit = {
val request = Request("/")
val response = await(handler.apply(request))
assert(response.status == Status.Ok)
val responseBody = response.contentString
assert(responseBody.startsWith(message))
}
}

0 comments on commit ce783a3

Please sign in to comment.