BitoAgent commented on code in PR #13786:
URL: https://github.com/apache/dubbo/pull/13786#discussion_r1573855670
##########
dubbo-rpc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/h12/grpc/GrpcHttp2ServerTransportListener.java:
##########
@@ -145,39 +146,31 @@ public void onMessage(InputStream inputStream) {
private class DetermineMethodDescriptorListener implements
StreamingDecoder.FragmentListener {
- @Override
- public void onFragmentMessage(InputStream rawMessage) {}
-
@Override
public void onClose() {
getStreamingDecoder().close();
}
@Override
- public void onFragmentMessage(InputStream dataHeader, InputStream
rawMessage) {
+ public void onFragmentMessage(InputStream rawMessage) {
try {
- ByteArrayOutputStream merged =
- new ByteArrayOutputStream(dataHeader.available() +
rawMessage.available());
- StreamUtils.copy(dataHeader, merged);
- byte[] data = StreamUtils.readBytes(rawMessage);
-
RpcInvocationBuildContext context = getContext();
if (null == context.getMethodDescriptor()) {
-
context.setMethodDescriptor(DescriptorUtils.findTripleMethodDescriptor(
- context.getServiceDescriptor(),
context.getMethodName(), data));
+ byte[] data = StreamUtils.readBytes(rawMessage);
+ MethodDescriptor methodDescriptor =
DescriptorUtils.findTripleMethodDescriptor(
+ context.getServiceDescriptor(),
context.getMethodName(), data);
+ context.setMethodDescriptor(methodDescriptor);
setHttpMessageListener(GrpcHttp2ServerTransportListener.super.buildHttpMessageListener());
// replace decoder
GrpcCompositeCodec grpcCompositeCodec =
(GrpcCompositeCodec) context.getHttpMessageDecoder();
- MethodMetadata methodMetadata =
context.getMethodMetadata();
-
grpcCompositeCodec.setDecodeTypes(methodMetadata.getActualRequestTypes());
- grpcCompositeCodec.setEncodeTypes(new Class[]
{methodMetadata.getActualResponseType()});
+ grpcCompositeCodec.loadPackableMethod(methodDescriptor);
getServerChannelObserver().setResponseEncoder(grpcCompositeCodec);
+ rawMessage = new ByteArrayInputStream(data);
}
- merged.write(data);
- getHttpMessageListener().onMessage(new
ByteArrayInputStream(merged.toByteArray()));
+ getStreamingDecoder().invokeListener(rawMessage);
Review Comment:
**Issue**: The refactor to streamline the DetermineMethodDescriptorListener
and the removal of unnecessary ByteArrayOutputStream usage are excellent for
performance. However, directly reading bytes from the rawMessage InputStream
without checking its availability or size could potentially lead to issues with
large messages. <br> **Fix**: Implement a check on the InputStream size or
availability before attempting to read bytes to ensure that the system can
handle large messages without running into memory issues. <br> **Code
Suggestion**:
```
if (rawMessage.available() > MAX_MESSAGE_SIZE) {
throw new IOException("Message size exceeds the maximum limit");
}
byte[] data = StreamUtils.readBytes(rawMessage);
context.setMethodDescriptor(DescriptorUtils.findTripleMethodDescriptor(context.getServiceDescriptor(),
context.getMethodName(), data));
```
##########
dubbo-rpc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/h12/grpc/GrpcCompositeCodecFactory.java:
##########
@@ -22,19 +22,14 @@
import org.apache.dubbo.remoting.http12.message.HttpMessageDecoderFactory;
import org.apache.dubbo.remoting.http12.message.HttpMessageEncoderFactory;
import org.apache.dubbo.remoting.http12.message.MediaType;
-import org.apache.dubbo.remoting.utils.UrlUtils;
import org.apache.dubbo.rpc.model.FrameworkModel;
@Activate
public class GrpcCompositeCodecFactory implements HttpMessageEncoderFactory,
HttpMessageDecoderFactory {
@Override
public HttpMessageCodec createCodec(URL url, FrameworkModel
frameworkModel, String mediaType) {
- String serializeName = UrlUtils.serializationOrDefault(url);
- WrapperHttpMessageCodec wrapperHttpMessageCodec = new
WrapperHttpMessageCodec(url, frameworkModel);
- wrapperHttpMessageCodec.setSerializeType(serializeName);
- ProtobufHttpMessageCodec protobufHttpMessageCodec = new
ProtobufHttpMessageCodec();
- return new GrpcCompositeCodec(protobufHttpMessageCodec,
wrapperHttpMessageCodec);
+ return new GrpcCompositeCodec(url, frameworkModel, mediaType);
Review Comment:
**Security Issue**: The method createCodec directly uses the URL,
frameworkModel, and mediaType parameters without validating them, potentially
leading to security vulnerabilities if these parameters are user-controlled or
can be manipulated. <br> **Fix**: Ensure the URL, frameworkModel, and mediaType
parameters are validated against expected values before using them in the
createCodec method. <br> **Code Suggestion**:
```
public HttpMessageCodec createCodec(URL url, FrameworkModel frameworkModel,
String mediaType) {
// Validate URL, frameworkModel, and mediaType parameters
if (url == null || frameworkModel == null || mediaType == null) {
throw new IllegalArgumentException("Invalid parameters for codec
creation.");
}
return new GrpcCompositeCodec(url, frameworkModel, mediaType);
}
```
##########
dubbo-remoting/dubbo-remoting-http12/src/main/java/org/apache/dubbo/remoting/http12/message/LengthFieldStreamingDecoder.java:
##########
@@ -130,16 +127,12 @@ private void deliver() {
}
private void processHeader() throws IOException {
- ByteArrayOutputStream bos = new
ByteArrayOutputStream(lengthFieldOffset + lengthFieldLength);
byte[] offsetData = new byte[lengthFieldOffset];
int ignore = accumulate.read(offsetData);
- bos.write(offsetData);
processOffset(new ByteArrayInputStream(offsetData), lengthFieldOffset);
byte[] lengthBytes = new byte[lengthFieldLength];
ignore = accumulate.read(lengthBytes);
- bos.write(lengthBytes);
requiredLength = bytesToInt(lengthBytes);
- this.dataHeader = new ByteArrayInputStream(bos.toByteArray());
// Continue reading the frame body.
state = DecodeState.PAYLOAD;
Review Comment:
**Security Issue**: The implementation of the method 'processHeader' does
not properly validate the length of the data being processed. This can lead to
buffer overflow vulnerabilities if the data exceeds expected bounds. <br>
**Fix**: Implement length checks for 'offsetData' and 'lengthBytes' to ensure
they do not exceed predefined safe limits. If the data exceeds these limits,
throw an IOException or a custom exception that can be properly handled. <br>
**Code Suggestion**:
```
private void processHeader() throws IOException {
byte[] offsetData = new byte[lengthFieldOffset];
int ignore = accumulate.read(offsetData);
if(offsetData.length > MAX_OFFSET_DATA_LENGTH) throw new
IOException("Offset data exceeds allowed limit.");
processOffset(new ByteArrayInputStream(offsetData), lengthFieldOffset);
byte[] lengthBytes = new byte[lengthFieldLength];
ignore = accumulate.read(lengthBytes);
if(lengthBytes.length > MAX_LENGTH_BYTES) throw new IOException("Length
bytes exceed allowed limit.");
requiredLength = bytesToInt(lengthBytes);
}
```
##########
dubbo-rpc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/h12/grpc/GrpcHttp2ServerTransportListener.java:
##########
@@ -145,39 +146,31 @@ public void onMessage(InputStream inputStream) {
private class DetermineMethodDescriptorListener implements
StreamingDecoder.FragmentListener {
- @Override
- public void onFragmentMessage(InputStream rawMessage) {}
-
@Override
public void onClose() {
getStreamingDecoder().close();
}
@Override
- public void onFragmentMessage(InputStream dataHeader, InputStream
rawMessage) {
+ public void onFragmentMessage(InputStream rawMessage) {
try {
- ByteArrayOutputStream merged =
- new ByteArrayOutputStream(dataHeader.available() +
rawMessage.available());
- StreamUtils.copy(dataHeader, merged);
- byte[] data = StreamUtils.readBytes(rawMessage);
-
RpcInvocationBuildContext context = getContext();
if (null == context.getMethodDescriptor()) {
-
context.setMethodDescriptor(DescriptorUtils.findTripleMethodDescriptor(
- context.getServiceDescriptor(),
context.getMethodName(), data));
+ byte[] data = StreamUtils.readBytes(rawMessage);
+ MethodDescriptor methodDescriptor =
DescriptorUtils.findTripleMethodDescriptor(
+ context.getServiceDescriptor(),
context.getMethodName(), data);
+ context.setMethodDescriptor(methodDescriptor);
setHttpMessageListener(GrpcHttp2ServerTransportListener.super.buildHttpMessageListener());
// replace decoder
GrpcCompositeCodec grpcCompositeCodec =
(GrpcCompositeCodec) context.getHttpMessageDecoder();
- MethodMetadata methodMetadata =
context.getMethodMetadata();
-
grpcCompositeCodec.setDecodeTypes(methodMetadata.getActualRequestTypes());
- grpcCompositeCodec.setEncodeTypes(new Class[]
{methodMetadata.getActualResponseType()});
+ grpcCompositeCodec.loadPackableMethod(methodDescriptor);
getServerChannelObserver().setResponseEncoder(grpcCompositeCodec);
+ rawMessage = new ByteArrayInputStream(data);
}
- merged.write(data);
- getHttpMessageListener().onMessage(new
ByteArrayInputStream(merged.toByteArray()));
+ getStreamingDecoder().invokeListener(rawMessage);
} catch (IOException e) {
throw new DecodeException(e);
}
Review Comment:
**Security Issue**: The code does not sanitize or validate input before
processing it, which may lead to injection vulnerabilities, such as SQL
Injection, Command Injection, or similar when the input is used in a
security-sensitive context. <br> **Fix**: Implement input validation and
sanitization to ensure the rawMessage data is safe to process. Use a whitelist
approach where only known good data is accepted. <br> **Code Suggestion**:
```
InputStream validatedStream = sanitizeInputStream(rawMessage);
if (validateInputStream(validatedStream)) {
byte[] data = StreamUtils.readBytes(validatedStream);
MethodDescriptor methodDescriptor =
DescriptorUtils.findTripleMethodDescriptor(context.getServiceDescriptor(),
context.getMethodName(), data);
context.setMethodDescriptor(methodDescriptor);
}
```
##########
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);
+ }
+ handler.setHttpMessageDecoder(grpcCompositeCodec);
+ handler.setHttpMessageEncoder(grpcCompositeCodec);
Review Comment:
**Security Issue**: The use of findMethodDescriptor without validating the
method's name, service descriptor, or the stub flag may lead to the execution
of unexpected methods if the input parameters can be influenced externally.
<br> **Fix**: Validate the method name, service descriptor, and stub flag
before using them to find and set the MethodDescriptor. <br> **Code
Suggestion**:
```
+ if (DescriptorUtils.validateMethodName(handler.getMethodName()) &&
+
DescriptorUtils.validateServiceDescriptor(handler.getServiceDescriptor()) &&
+ handler.isHasStub()) {
+ MethodDescriptor methodDescriptor =
DescriptorUtils.findMethodDescriptor(
+ handler.getServiceDescriptor(), handler.getMethodName(),
handler.isHasStub());
+ if (methodDescriptor != null) {
+ handler.setMethodDescriptor(methodDescriptor);
+ grpcCompositeCodec.loadPackableMethod(methodDescriptor);
+ }
+ }
```
##########
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);
+ }
+ handler.setHttpMessageDecoder(grpcCompositeCodec);
+ handler.setHttpMessageEncoder(grpcCompositeCodec);
Review Comment:
**Optimization Issue**: The process of determining the HttpMessageCodec has
been refactored to directly use GrpcCompositeCodec and attempt to find and load
a MethodDescriptor. This approach assumes that the MethodDescriptor can always
be found and correctly loaded, which might not always be the case. This could
lead to scenarios where the codec is not correctly configured for the request,
impacting performance and correctness. <br> **Fix**: Add error handling and
checks around the MethodDescriptor finding and loading process. Ensure that
there is a fallback or error reporting mechanism in place if the
MethodDescriptor cannot be found or if the loading process fails, to maintain
the robustness and performance of the request handling. <br> **Code
Suggestion**:
```
+ try {
+ 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);
+ } else {
+ throw new
MethodDescriptorNotFoundException("MethodDescriptor not found for methodName: "
+ handler.getMethodName());
+ }
+ } catch (MethodDescriptorNotFoundException e) {
+ logger.error(e.getMessage());
+ // Handle error or fallback
+ }
```
--
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]
