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

["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