[GitHub] [kafka] divijvaidya commented on a diff in pull request #13100: MINOR: add size check for tagged fields

[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