You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2023/01/10 07:50:15 UTC
[GitHub] [kafka] showuon opened a new pull request, #13100: MINOR: add size check for tagged fields
showuon opened a new pull request, #13100:
URL: https://github.com/apache/kafka/pull/13100
Add size check for taggedFields of a tag, and add tests.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on pull request #13100: MINOR: add size check for tagged fields
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on PR #13100:
URL: https://github.com/apache/kafka/pull/13100#issuecomment-1437298488
@showuon Thanks for the PR. The new test is failing with
```
org.opentest4j.AssertionFailedError: expected: <Error reading array of size 2147483647, only 3 bytes available> but was: <Error reading field of size 2147483647, only 3 bytes available>
```
https://ci-builds.apache.org/job/Kafka/job/kafka-pr/job/PR-13100/3/testReport/org.apache.kafka.common.protocol.types/ProtocolSerializationTest/Build___JDK_11_and_Scala_2_13___testReadTaggedFieldsSizeTooLarge__/
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a diff in pull request #13100: MINOR: add size check for tagged fields
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on code in PR #13100:
URL: https://github.com/apache/kafka/pull/13100#discussion_r1105435108
##########
clients/src/main/java/org/apache/kafka/common/protocol/types/TaggedFields.java:
##########
@@ -100,6 +100,13 @@ public NavigableMap<Integer, Object> read(ByteBuffer buffer) {
}
prevTag = tag;
int size = ByteUtils.readUnsignedVarint(buffer);
+ if (size < 0 && isNullable())
+ return null;
+ else if (size < 0)
+ throw new SchemaException("field size " + size + " cannot be negative");
+ if (size > buffer.remaining())
+ throw new SchemaException("Error reading field of size " + size + ", only " + buffer.remaining() + " bytes available");
Review Comment:
@divijvaidya , I was following your suggestion to do the documentType refactor like this https://github.com/apache/kafka/compare/trunk...showuon:taggedFields_temp?expand=1 , and then, I found in `taggedFields`, the size validation is in nested way. So, that makes the validation framework doesn't be that useful. Besides, after the change, the child of `DocumentType` can still override the `read` method if it wants (inherited from parent `Type` class), which makes it not useful. So I reverted back to the original change, and add a comment to the `read` method. Does that make sense?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #13100: MINOR: add size check for tagged fields
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #13100:
URL: https://github.com/apache/kafka/pull/13100#discussion_r1065689076
##########
clients/src/main/java/org/apache/kafka/common/protocol/types/TaggedFields.java:
##########
@@ -100,6 +100,13 @@ public NavigableMap<Integer, Object> read(ByteBuffer buffer) {
}
prevTag = tag;
int size = ByteUtils.readUnsignedVarint(buffer);
+ if (size < 0 && isNullable())
Review Comment:
`TaggedFields` are never nullable. We can combine this with if clause below.
##########
clients/src/main/java/org/apache/kafka/common/protocol/types/TaggedFields.java:
##########
@@ -100,6 +100,13 @@ public NavigableMap<Integer, Object> read(ByteBuffer buffer) {
}
prevTag = tag;
int size = ByteUtils.readUnsignedVarint(buffer);
+ if (size < 0 && isNullable())
+ return null;
+ else if (size < 0)
+ throw new SchemaException("field size " + size + " cannot be negative");
+ if (size > buffer.remaining())
+ throw new SchemaException("Error reading field of size " + size + ", only " + buffer.remaining() + " bytes available");
Review Comment:
This validation needs to be performed for every Type of subclass of `DocumentedType`. We can potentially abstract these checks out to a method.
I would suggest the following:
1. In the base class `DocumentedType` don't make read() an abstract method.
2. The read() method in `DocumentedType` should look like this:
```
read() {
// this method is implemented in child classes
calculateSizeToRead()
// perform size validation
if (size < 0)
.. blah blah.. // perform the if else size validation you are doing here
// call child class method to actually read
_read()
}
```
This would ensure that if a new `type` is added in future, the author won't forget to perform size checks.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon merged pull request #13100: MINOR: add size check for tagged fields
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon merged PR #13100:
URL: https://github.com/apache/kafka/pull/13100
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #13100: MINOR: add size check for tagged fields
Posted by GitBox <gi...@apache.org>.
showuon commented on PR #13100:
URL: https://github.com/apache/kafka/pull/13100#issuecomment-1376856241
@ijuma @mimaison , please take a look. Thanks.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #13100: MINOR: add size check for tagged fields
Posted by GitBox <gi...@apache.org>.
divijvaidya commented on code in PR #13100:
URL: https://github.com/apache/kafka/pull/13100#discussion_r1065703012
##########
clients/src/main/java/org/apache/kafka/common/protocol/types/TaggedFields.java:
##########
@@ -100,6 +100,13 @@ public NavigableMap<Integer, Object> read(ByteBuffer buffer) {
}
prevTag = tag;
int size = ByteUtils.readUnsignedVarint(buffer);
+ if (size < 0 && isNullable())
+ return null;
+ else if (size < 0)
+ throw new SchemaException("field size " + size + " cannot be negative");
+ if (size > buffer.remaining())
+ throw new SchemaException("Error reading field of size " + size + ", only " + buffer.remaining() + " bytes available");
Review Comment:
This validation needs to be performed for every Type of subclass of `DocumentedType`. We can potentially abstract these checks out to a method.
I would suggest the following:
1. In the base class `DocumentedType` don't make read() an abstract method.
2. The read() method in `DocumentedType` should look like this:
```
read() {
// this method is implemented in child classes
calculateSizeToRead()
// perform size validation
if (size < 0)
.. blah blah.. // perform the if else size validation you are doing here
// call child class method to actually read
_read()
}
abstract _read()
abstract _calculateSize()
```
This would ensure that if a new `type` is added in future, the author won't forget to perform size checks.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a diff in pull request #13100: MINOR: add size check for tagged fields
Posted by GitBox <gi...@apache.org>.
showuon commented on code in PR #13100:
URL: https://github.com/apache/kafka/pull/13100#discussion_r1066632142
##########
clients/src/main/java/org/apache/kafka/common/protocol/types/TaggedFields.java:
##########
@@ -100,6 +100,13 @@ public NavigableMap<Integer, Object> read(ByteBuffer buffer) {
}
prevTag = tag;
int size = ByteUtils.readUnsignedVarint(buffer);
+ if (size < 0 && isNullable())
+ return null;
+ else if (size < 0)
+ throw new SchemaException("field size " + size + " cannot be negative");
+ if (size > buffer.remaining())
+ throw new SchemaException("Error reading field of size " + size + ", only " + buffer.remaining() + " bytes available");
Review Comment:
That's a good point. I'll work on it. Thanks.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #13100: MINOR: add size check for tagged fields
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on PR #13100:
URL: https://github.com/apache/kafka/pull/13100#issuecomment-1437718642
@mimaison , sorry, my bad, PR updated. Thanks.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org