-
-
Notifications
You must be signed in to change notification settings - Fork 15.9k
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 support for shading native libraries which was broken in b818852c… #8091
Conversation
@cchantep please check as well ... thanks! |
Motivation: b818852 broke support for shading the native libraries in netty as it missed to respect the package prefix that is used when shading. Modifications: Correctly respect package prefix for constructor argument and include the used classname when logging that we could not find the constructor. Result: Be able to shade native libraries of netty again.
3ee47d0
to
bb395ad
Compare
I will work on a integration test for this one so we will not break it again |
datagramSocketAddrMethodId = (*env)->GetMethodID(env, datagramSocketAddressClass, "<init>", "(Ljava/lang/String;IILio/netty/channel/unix/DatagramSocketAddress;)V"); | ||
|
||
// Respect shading... | ||
char parameters[1024] = {0}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be safer to calculate the length instead of using an arbitrary size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could... that said if 1024 is not enough I would be really surprised
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking in terms of buffer overflow but I assume snprintf guards against that? couldn't we do a simple check that nettyClassName does not exceed 1024 and if it does, throw illegalargumentexception just to be on the safe side?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@normanmaurer poke
@normanmaurer do you know whether it's published on snapshot repo ? To check with CI? |
…db0ee56ce3cf939e2faa538d519baabd.
Motivation:
b818852 broke support for shading the native libraries in netty as it missed to respect the package prefix that is used when shading.
Modifications:
Correctly respect package prefix for constructor argument and include the used classname when logging that we could not find the constructor.
Result:
Be able to shade native libraries of netty again.