2

I have a class that can be instantiated only once for any given thread, through the use of a ThreadLocal, for instance:

public class MyClass {
    private static final ThreadLocal<MyClass> classInstance =
        new ThreadLocal<MyClass>() {
            @Override protected MyClass initialValue() {
                return new MyClass();
        }
    };

    private MyClass() {
    }

    public static MyClass getInstance() {
        return classInstance.get();
    }    
}

Now, I want these thread-specific instances to be accessed by another thread, so I end up with that kind of solution: https://stackoverflow.com/a/5180323/1768736

This solution is to use a Map (I would go for a ConcurrentHashMap), using as key a Thread-specific ID (for instance, a Map<long, MyClass>). I was considering to use Thread.currentThread().getId() as a key (providing some mechanisms to deal with the fact that a Thread ID can be reused).

But it means that I need to expose this Thread ID, for instance:

public class MyClass {
    ...
    public long getId() {
        //return the key associated with this MyClass instance in the Map. 
        //in my case, this would be the ID of the Thread
    }
    public static MyClass getMyClass(long threadId) {
        //allow a Thread to access the MyClass instance of another Thread by providing an ID, 
        //in my case, that would be the Thread ID
    } 
}

So my question is: is it a bad practice to expose the ID of a Thread (the one returned by Thread.getId()), or do I have nothing to worry about? I don't know why, but my gut tells me I shouldn't do that.

Community
  • 1
  • 1
FBB
  • 1,414
  • 2
  • 17
  • 29
  • 2
    i would say that if you are exposing a "thread local" instance to other threads, then you probably have a weak design. you probably need to re-think the overall design and no link instances to threads. – jtahlborn Jul 06 '13 at 14:11
  • 1
    as for the "security" issue, is your code running in an environment in which security of code is an issue (i.e. you need to be concerned about malicious code)? – jtahlborn Jul 06 '13 at 14:11
  • 1
    Exposing thread ID would be way down on the list of possible security problems. – Hot Licks Jul 06 '13 at 16:22
  • @jtahlborn: I think it is not what you are saying, but just to be sure, my point is exactly to not expose a `ThreadLocal`, but rather to use a `Map` with the Thread ID as key. Now, if you are saying that I should not expose a "thread local-like", as I don't see why, I give you my use case: I have threads, each using their "thread local-like" instance to perform a task, and I have a master thread, which can choose to kill the task of another thread; the master thread will acquire the "thread local-like" instance of that other thread, and call a "kill" method on it. So how is this a bad design? – FBB Jul 06 '13 at 20:48
  • why wouldn't the master just call a method on the other instance? it sounds like you need to find the other task, grab it's thread id, use thread id to find another instance frmo which to kill the task. if you have the task, why not expose the kill method directly. – jtahlborn Jul 07 '13 at 12:49
  • I agree with you, but my point is exactly to "find the other task", because I actually don't have it (my explanation of a "master" thread was unclear). And to find it, I need an ID. So I was considering using the thread ID. – FBB Jul 08 '13 at 11:22

2 Answers2

3

Since Thread Id cannot be used to kill the thread (directly), it is safe to use. As applications can assign thread ids at the time of creating threads, I believe its usage is only to help you debug the application when you are looking at logs, thread-dumps, etc.

Also, as this link explains, it is not hard to get list of threads that are running in JVM. As long as you are aware of the fact that thread ids can get re-used after a thread has died, and your app's logic takes care of that, you should be alright.

Community
  • 1
  • 1
Wand Maker
  • 18,476
  • 8
  • 53
  • 87
  • But what if this thread ID goes across the network somehow? A remote user might not be able to get the list of threads running in a JVM. I know it's rather unlikely that this could help an attack, but still... – einpoklum Jul 07 '13 at 14:48
  • @einpoklum: yep, this is the kind of things I am afraid of, but as I don't know any potential issues... – FBB Jul 07 '13 at 22:22
0

I assume you are concerned about mobile-code security. It wouldn't appear to be the sort of thing that would introduce a vulnerability otherwise (I.D. reuse as a very long shot).

If you are worried about mobile-code security, then you want to avoid global and thread-global state anyway. You would anyway. Carrying on regardless...

From a security point of view Maps cause problems, particularly if you use non-values such as Thread for keys. A malicious subclass can pretend to be not equal to itself, equal to a different thread or acquire a Thread through Object.equals(Object). An IdentityHashMap would be ideal here, only you probably want a WeakHashMap. (I don't know why WeakHashMap uses key equality behaviour defined in Map rather than that in IdentityHashMap. A ConcurrentWeakIdentityHashMap (a "star map") would be excellent.)

Without modifying Thread, you probably want to introduce a key object that wraps thread giving the required behaviour.

final class Key {
    private final int hash;
    private final WeakReference<Thread> ref;
    public Key(Thread thread) {
        this.hash = System.identityHashCode(thread);
        this.ref = new WeakReference<Thread>();
    }
    @Override public int hashCode() {
        return hash;
    }
    @Override public boolean equals(Object obj) {
        if (this == obj) return true;
        if (!(obj instanceof Key)) return false;
        Key other = (Key)obj;
        Thread thread = this.ref.get();
        return thread != null && thread == other.ref.get(); 
    }
}

(You will probably want to queue references so entries can be removed.)

Thread.getAllStackStraces has a security check, so untrusted code can't get them all. Similar calls limit execution to accessible thread groups.

Tom Hawtin - tackline
  • 145,806
  • 30
  • 211
  • 305
  • So if I understand you correctly, using the ThreadID in my `Map` would be fine, is that right? – FBB Jul 12 '13 at 14:43
  • I guess so long as you aren't dealing with untrusted code (subclass `Thread`, override (the incorrectly capitalised) `getId`, Bob is your aunt). However, it may leak (presumably `Thread` isn't the key to minimise this) and you're dealing with globals. – Tom Hawtin - tackline Jul 12 '13 at 15:27