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

rpc: more accurate checking of callback/subpub method signatures #27287

Merged
merged 3 commits into from
May 17, 2023

Conversation

stephenfire
Copy link
Contributor

The first argument of a Pub/Sub function should not be a pointer to context.Context, otherwise it will be registered as a subscription and its corresponding callback.hasCtx==false. This results in the parameter not having the correct value when called.

The problem description of the old code is as follows:

  1. Suppose we define a subscription method, the first parameter is *context.Context.
  2. When executing serviceRegistry.registerName for service registration, the isContextType method returns true, and isPubSub returns true, making callback.isSubscribe==true.
  3. In the makeArgTypes method, because the first parameter Type!=contextType, the previously created callback.hasCtx==false.
  4. In the callback.Call method, because callback.hasCtx==false, the correct context.Context cannot be passed into the corresponding method.

@stephenfire stephenfire requested review from fjl and holiman as code owners May 17, 2023 03:49
@stephenfire stephenfire changed the title rpc: the first argument of a Pub/Sub function should be a context.Context rpc: more accurate checking of callback/subpub method signatures May 17, 2023
@stephenfire
Copy link
Contributor Author

Make sure callback/subscription returns error instead of *error.

If a callback or subscription method returns *error, it will still be judged as callback or subscription, and the position of the returned *error will be recorded in callback.errPos. In callback.Call, when the corresponding position value of callback.errPos is not nil, executing err := results[c.errPos].Interface().(error) will cause panic of "interface conversion: *error is not error: missing method Error"

Copy link
Contributor

@fjl fjl left a comment

Choose a reason for hiding this comment

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

Thanks!

@fjl fjl added this to the 1.11.7 milestone May 17, 2023
@fjl fjl merged commit 84c3799 into ethereum:master May 17, 2023
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
)

This changes the RPC server to ignore methods using *context.Context as parameter
and *error as return value type. Methods with such types would crash the server when
called.
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
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