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

fix(java): ThreadPoolFury and ThreadLocalFury concurrency security issue #1525

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

LiangliangSui
Copy link
Contributor

What does this PR do?

Fix potential thread safety issues with ThreadPoolFury and ThreadLocalFury.

Related issues

#1524

Does this PR introduce any user-facing change?

  • Does this PR introduce any public API change?
  • Does this PR introduce any binary protocol compatibility change?

Benchmark

Signed-off-by: LiangliangSui <[email protected]>
@LiangliangSui LiangliangSui changed the title fix(java): ThreadPoolFury and ThreadLocalFury concurrency security issues fix(java): ThreadPoolFury and ThreadLocalFury concurrency security issue Apr 16, 2024
@LiangliangSui LiangliangSui reopened this Apr 16, 2024
@@ -122,6 +124,20 @@ private void addFury() {
}

void setFactoryCallback(Consumer<Fury> factoryCallback) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method should not be called concurently, users should invoke regitster and other fury config methods when creating ThreadSafeFury. It's not necessary to add a lock

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, all our register and config functions should be called when ThreadSafeFury is being created, but the purpose of adding the processCallback function should be contrary to this design. The processCallback interface allows register or config operations to be performed after created ThreadSafeFury.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ThreadSafeFury safeFury = Fury.builder()
    .requireClassRegistration(true)
    .buildThreadSafeFury();
safeFury.register(A.class);
safeFury.register(B.class);

// Thread A
new Thread(() -> {
  safeFury.serialize(new A());
}).start();

// Thread B
new Thread(() -> {
  // processCallback(fury -> fury.register(clz));
  safeFury.register(C.class);
  safeFury.serialize(new C());
}).start();

As shown in the above Demo, how can we avoid safeFury.register(C.class) being called in Thread B.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we should add code to avoid this, such as capture Thread, and check thread in later call

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a feasible approach, but I think the methods that implement the ThreadSafeFury interface should be thread-safe, and all methods in ThreadSafeFury should be allowed to be called in any thread.

@chaokunyang
Copy link
Collaborator

If we allow register classes currently, we do need to make those methods Thread safe.
But it's complex that we register some classes, serialize some objects, Then serialize other classes

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

Successfully merging this pull request may close these issues.

2 participants