BitoAgent commented on code in PR #13786:
URL: https://github.com/apache/dubbo/pull/13786#discussion_r1552429021
##########
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:
**Issue**: The removal of 'ByteArrayOutputStream' and direct processing of
byte arrays from the 'InputStream' is a significant change that could impact
how message headers are handled. Ensure that this change does not affect the
protocol's expected behavior, especially in edge cases with large headers or
payloads. <br> **Fix**: Thoroughly test the new implementation with various
message sizes and formats to ensure compatibility with the expected HTTP/2
framing and encoding behaviors. <br> **Code Suggestion**:
```
- 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());
```
##########
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 increases the method's exposure, potentially allowing unintended
interactions. Review if this change is necessary for the intended design and
consider potential security implications. <br> **Fix**: If the method needs to
be exposed publicly for legitimate reasons, ensure that its usage does not
introduce security vulnerabilities or allow misuse. <br> **Code Suggestion**:
```
- protected void invokeListener(InputStream inputStream) {
+ public void invokeListener(InputStream inputStream) {
```
--
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]
