search

SpigotMC / BungeeCord

BungeeCord, the 6th in a generation of server portal suites. Efficiently proxies and maintains connections and transport between multiple Minecraft servers.
https://www.spigotmc.org/go/bungeecord
Other
1.58k stars 1.11k forks source link

Potential Thread Safety Issues #2932

Open A248 opened 4 years ago

A248 commented 4 years ago

It seems to me a lot of parts of the proxy are unsychronised where I would think mutual exclusion would be required.

  1. Plugin#getExecutorService. Although deprecated, if called multiple times, it's possible multiple ExecutorServices could be instantiated. Seeing as this method is used by BungeeScheduler, this could cause a resource leak should plugins use scheduling methods concurrently.

  2. BungeeSecurityManager: The HashSet seen is unsynchronised, but could be accessed and mutated by multiple threads.

  3. PluginManager and everything about it. It has multiple non-thread safe collections in its fields. Though it's unlikely plugins would register commands or listeners asynchronously, this is not something I think should be ignored.

  4. ConciseFormatter uses SimpleDateFormat, which is not thread safe, in what appears to be an unsynchronised manner. I am not aware of any synchronisation performed by java.util.logging.Formatter. Plugins logging concurrently is definitely a possibility. Though the repercussions of this might only be an infrequent oddly-formatted date, better to follow best practices and abide by thread safe access. Sometimes not doing so incurs unexpected consequences. FileHandler#publish/StreamHandler#publish, which uses the Formatter, is already synchronized.

Although some of these situations may be rather minor, I would presume it's in BungeeCord's best interest to maintain integrity at all times, even where small things such as registering commands and listeners or executing tasks are concerned.

linsaftw commented 4 years ago

Hi, how about making a PR? I made a fork and if you like we can implement and test this changes on it 👍

A248 commented 4 years ago

I realised Formatter#format does not need to be thread safe.

linsaftw commented 4 years ago

Hi, i commented on Discord this but i comment here too; Maybe you can make the methods synchronized instead of adding a synchronized block.