BitoAgent commented on code in PR #13786:
URL: https://github.com/apache/dubbo/pull/13786#discussion_r1573356158
##########
dubbo-remoting/dubbo-remoting-http12/src/main/java/org/apache/dubbo/remoting/http12/message/LengthFieldStreamingDecoder.java:
##########
@@ -167,8 +160,8 @@ private void processBody() throws IOException {
requiredLength = lengthFieldOffset + lengthFieldLength;
}
- protected void invokeListener(InputStream inputStream) {
- this.listener.onFragmentMessage(dataHeader, inputStream);
+ public void invokeListener(InputStream inputStream) {
+ this.listener.onFragmentMessage(inputStream);
Review Comment:
**Issue**: Changing the visibility of invokeListener from protected to
public may expose internal mechanisms unnecessarily. <br> **Fix**: Review the
necessity of this visibility change. If external access is not needed, revert
to protected. <br> **Code Suggestion**:
```
- protected void invokeListener(InputStream inputStream) {
+ private void invokeListener(InputStream inputStream) {
```
##########
dubbo-rpc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/h12/grpc/GrpcRequestHandlerMapping.java:
##########
@@ -42,9 +43,16 @@ protected boolean supportContentType(String contentType) {
@Override
protected void determineHttpMessageCodec(RequestHandler handler, URL url,
HttpRequest request) {
- HttpMessageCodec codec = CODEC_FACTORY.createCodec(url,
getFrameworkModel(), request.contentType());
- handler.setHttpMessageDecoder(codec);
- handler.setHttpMessageEncoder(codec);
+ GrpcCompositeCodec grpcCompositeCodec =
+ (GrpcCompositeCodec) CODEC_FACTORY.createCodec(url,
getFrameworkModel(), request.contentType());
+ MethodDescriptor methodDescriptor =
DescriptorUtils.findMethodDescriptor(
+ handler.getServiceDescriptor(), handler.getMethodName(),
handler.isHasStub());
+ if (methodDescriptor != null) {
+ handler.setMethodDescriptor(methodDescriptor);
+ grpcCompositeCodec.loadPackableMethod(methodDescriptor);
Review Comment:
**Security Issue**: The code dynamically loads a MethodDescriptor based on
request data without proper validation, potentially allowing for unauthorized
method execution or access control bypass. <br> **Fix**: Implement strict
validation of the request data used to find the MethodDescriptor. Ensure that
the method access is authorized for the requesting user before setting the
MethodDescriptor. <br> **Code Suggestion**:
```
Integrate strict request data validation before loading the
MethodDescriptor to ensure only authorized access. Include checks for user
authorization in relation to the requested MethodDescriptor.
```
##########
dubbo-remoting/dubbo-remoting-http12/src/main/java/org/apache/dubbo/remoting/http12/message/LengthFieldStreamingDecoder.java:
##########
@@ -167,8 +160,8 @@ private void processBody() throws IOException {
requiredLength = lengthFieldOffset + lengthFieldLength;
}
- protected void invokeListener(InputStream inputStream) {
- this.listener.onFragmentMessage(dataHeader, inputStream);
+ public void invokeListener(InputStream inputStream) {
+ this.listener.onFragmentMessage(inputStream);
Review Comment:
**Security Issue**: Changing the visibility of the invokeListener method
from protected to public without proper validation can expose internal
processing mechanisms to unintended usage, potentially leading to unauthorized
access or denial of service. <br> **Fix**: Reinstate the method's protected
visibility and ensure that any external calls to this method are properly
authenticated and validated to prevent unauthorized access. <br> **Code
Suggestion**:
```
- protected void invokeListener(InputStream inputStream) {
+ private void invokeListener(InputStream inputStream) {
```
##########
dubbo-rpc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/h12/grpc/GrpcCompositeCodec.java:
##########
@@ -16,123 +16,103 @@
*/
package org.apache.dubbo.rpc.protocol.tri.h12.grpc;
+import org.apache.dubbo.common.URL;
+import org.apache.dubbo.common.config.ConfigurationUtils;
+import org.apache.dubbo.common.io.StreamUtils;
+import org.apache.dubbo.common.utils.ArrayUtils;
import org.apache.dubbo.remoting.http12.exception.DecodeException;
import org.apache.dubbo.remoting.http12.exception.EncodeException;
import org.apache.dubbo.remoting.http12.message.HttpMessageCodec;
import org.apache.dubbo.remoting.http12.message.MediaType;
+import org.apache.dubbo.rpc.model.FrameworkModel;
+import org.apache.dubbo.rpc.model.MethodDescriptor;
+import org.apache.dubbo.rpc.model.PackableMethod;
+import org.apache.dubbo.rpc.model.PackableMethodFactory;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.nio.charset.Charset;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
-import com.google.protobuf.Message;
-
-import static
org.apache.dubbo.common.constants.CommonConstants.PROTOBUF_MESSAGE_CLASS_NAME;
+import static org.apache.dubbo.common.constants.CommonConstants.DEFAULT_KEY;
+import static
org.apache.dubbo.common.constants.CommonConstants.DUBBO_PACKABLE_METHOD_FACTORY;
public class GrpcCompositeCodec implements HttpMessageCodec {
- private final ProtobufHttpMessageCodec protobufHttpMessageCodec;
+ private static final String PACKABLE_METHOD_CACHE =
"PACKABLE_METHOD_CACHE";
- private final WrapperHttpMessageCodec wrapperHttpMessageCodec;
+ private final URL url;
- public GrpcCompositeCodec(
- ProtobufHttpMessageCodec protobufHttpMessageCodec,
WrapperHttpMessageCodec wrapperHttpMessageCodec) {
- this.protobufHttpMessageCodec = protobufHttpMessageCodec;
- this.wrapperHttpMessageCodec = wrapperHttpMessageCodec;
- }
+ private final FrameworkModel frameworkModel;
+
+ private final String mediaType;
- public void setEncodeTypes(Class<?>[] encodeTypes) {
- this.wrapperHttpMessageCodec.setEncodeTypes(encodeTypes);
+ private PackableMethod packableMethod;
+
+ public GrpcCompositeCodec(URL url, FrameworkModel frameworkModel, String
mediaType) {
+ this.url = url;
+ this.frameworkModel = frameworkModel;
+ this.mediaType = mediaType;
}
- public void setDecodeTypes(Class<?>[] decodeTypes) {
- this.wrapperHttpMessageCodec.setDecodeTypes(decodeTypes);
+ public void loadPackableMethod(MethodDescriptor methodDescriptor) {
+ if (methodDescriptor instanceof PackableMethod) {
+ packableMethod = (PackableMethod) methodDescriptor;
+ return;
+ }
+ Map<MethodDescriptor, PackableMethod> cacheMap =
(Map<MethodDescriptor, PackableMethod>) url.getServiceModel()
+ .getServiceMetadata()
+ .getAttributeMap()
+ .computeIfAbsent(PACKABLE_METHOD_CACHE, k -> new
ConcurrentHashMap<>());
+ packableMethod = cacheMap.computeIfAbsent(methodDescriptor, md ->
frameworkModel
+ .getExtensionLoader(PackableMethodFactory.class)
+
.getExtension(ConfigurationUtils.getGlobalConfiguration(url.getApplicationModel())
+ .getString(DUBBO_PACKABLE_METHOD_FACTORY, DEFAULT_KEY))
+ .create(methodDescriptor, url, mediaType));
}
@Override
public void encode(OutputStream outputStream, Object data, Charset
charset) throws EncodeException {
- // protobuf
- // TODO int compressed =
Identity.MESSAGE_ENCODING.equals(requestMetadata.compressor.getMessageEncoding())
? 0 :
- // 1;
try {
- int compressed = 0;
- outputStream.write(compressed);
- if (isProtobuf(data)) {
- ProtobufWriter.write(protobufHttpMessageCodec, outputStream,
data);
- return;
- }
- // wrapper
- wrapperHttpMessageCodec.encode(outputStream, data);
- } catch (IOException e) {
+ outputStream.write(0);
+ byte[] bytes = packableMethod.packResponse(data);
+ writeLength(outputStream, bytes.length);
+ outputStream.write(bytes);
Review Comment:
**Scalability Issue**: Serializing the response using
packableMethod.packResponse(data) inside the encode method without considering
the size of the data being serialized can lead to inefficient memory use and
potential OutOfMemoryError for large data sizes. <br> **Fix**: Before
serializing large data, consider checking the size and possibly breaking it
into smaller chunks, or using a streaming approach for serialization to manage
memory usage more efficiently. <br> **Code Suggestion**:
```
public void encode(OutputStream outputStream, Object data, Charset charset)
throws EncodeException {
try {
byte[] bytes = packableMethod.packResponse(data);
if (bytes.length > LARGE_DATA_THRESHOLD) {
// Implement logic to handle large data
handleLargeData(outputStream, bytes);
} else {
writeLength(outputStream, bytes.length);
outputStream.write(bytes);
}
} catch (Exception e) {
throw new EncodeException(e);
}
}
```
##########
dubbo-rpc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/TripleHeaderEnum.java:
##########
@@ -45,8 +45,7 @@ public enum TripleHeaderEnum {
SERVICE_GROUP("tri-service-group"),
SERVICE_TIMEOUT("tri-service-timeout"),
TRI_HEADER_CONVERT("tri-header-convert"),
- TRI_EXCEPTION_CODE("tri-exception-code"),
- ;
+ TRI_EXCEPTION_CODE("tri-exception-code");
static final Map<String, TripleHeaderEnum> enumMap = new HashMap<>();
Review Comment:
**Scalability Issue**: Removal of TRI_EXCEPTION_CODE enum value
simplification might impact error handling capabilities, indirectly affecting
the system's scalability by potentially increasing error rates or complicating
error diagnostics. <br> **Fix**: If the removal was intentional and handled
elsewhere, ensure robust error handling mechanisms are in place to support
scalability. <br> **Code Suggestion**:
```
Ensure error handling is robust, possibly by adding or enhancing mechanisms
elsewhere to mitigate the impact of this change.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
