You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "bachmanity1 (via GitHub)" <gi...@apache.org> on 2023/02/16 17:04:21 UTC
[GitHub] [kafka] bachmanity1 opened a new pull request, #13261: MINOR: after reading BYTES type it's possible to access data beyond its size
bachmanity1 opened a new pull request, #13261:
URL: https://github.com/apache/kafka/pull/13261
After reading data of type `BYTES`, `COMPACT_BYTES`, `NULLABLE_BYTES` or `COMPACT_NULLABLE_BYTES` returned `ByteBuffer` might have a capacity that is larger than its limit, thus these data types may access bytes that lies beyond its size by increasing limit of the returned `ByteBuffer`. I guess this is not very critical but I think it would be good to restrict increasing limit of the returned `ByteBuffer` by making its capacity strictly equal to its limit. I think someone might unintentionally mishandle these data types and accidentally mess up data in the `ByteBuffer` from which they were read.
### 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] bachmanity1 commented on a diff in pull request #13261: MINOR: after reading BYTES type it's possible to access data beyond its size
Posted by "bachmanity1 (via GitHub)" <gi...@apache.org>.
bachmanity1 commented on code in PR #13261:
URL: https://github.com/apache/kafka/pull/13261#discussion_r1114028635
##########
clients/src/main/java/org/apache/kafka/common/protocol/types/Type.java:
##########
@@ -688,8 +688,10 @@ public Object read(ByteBuffer buffer) {
if (size > buffer.remaining())
throw new SchemaException("Error reading bytes of size " + size + ", only " + buffer.remaining() + " bytes available");
+ int limit = buffer.limit();
+ buffer.limit(buffer.position() + size);
Review Comment:
I introduced a new variable `newPosition`
--
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 #13261: MINOR: after reading BYTES type it's possible to access data beyond its size
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon merged PR #13261:
URL: https://github.com/apache/kafka/pull/13261
--
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] bachmanity1 commented on pull request #13261: MINOR: after reading BYTES type it's possible to access data beyond its size
Posted by "bachmanity1 (via GitHub)" <gi...@apache.org>.
bachmanity1 commented on PR #13261:
URL: https://github.com/apache/kafka/pull/13261#issuecomment-1433794388
@showuon @C0urante could you please review this? 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] bachmanity1 commented on pull request #13261: MINOR: after reading BYTES type it's possible to access data beyond its size
Posted by "bachmanity1 (via GitHub)" <gi...@apache.org>.
bachmanity1 commented on PR #13261:
URL: https://github.com/apache/kafka/pull/13261#issuecomment-1441126265
The CI test failures look unrelated to this PR
--
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 #13261: MINOR: after reading BYTES type it's possible to access data beyond its size
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on PR #13261:
URL: https://github.com/apache/kafka/pull/13261#issuecomment-1441133511
Failed tests are unrelated
```
Build / JDK 11 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsWithCustomForwardingAdminIntegrationTest.testReplicationIsCreatingTopicsUsingProvidedForwardingAdmin()
Build / JDK 8 and Scala 2.12 / kafka.admin.ReassignPartitionsIntegrationTest.testReassignment(String).quorum=kraft
Build / JDK 17 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.DedicatedMirrorIntegrationTest.testMultiNodeCluster()
Build / JDK 17 and Scala 2.13 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsWithCustomForwardingAdminIntegrationTest.testCreatePartitionsUseProvidedForwardingAdmin()
Build / JDK 17 and Scala 2.13 / kafka.server.KafkaServerKRaftRegistrationTest.[1] Type=ZK, Name=testRegisterZkBrokerInKraft, MetadataVersion=3.4-IV0, Security=PLAINTEXT
```
--
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 #13261: MINOR: after reading BYTES type it's possible to access data beyond its size
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on code in PR #13261:
URL: https://github.com/apache/kafka/pull/13261#discussion_r1113935551
##########
clients/src/main/java/org/apache/kafka/common/protocol/types/Type.java:
##########
@@ -688,8 +688,10 @@ public Object read(ByteBuffer buffer) {
if (size > buffer.remaining())
throw new SchemaException("Error reading bytes of size " + size + ", only " + buffer.remaining() + " bytes available");
+ int limit = buffer.limit();
+ buffer.limit(buffer.position() + size);
Review Comment:
nit: Since we will use `buffer.position() + size` twice, should we make it as a variable?
--
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] C0urante commented on pull request #13261: MINOR: after reading BYTES type it's possible to access data beyond its size
Posted by "C0urante (via GitHub)" <gi...@apache.org>.
C0urante commented on PR #13261:
URL: https://github.com/apache/kafka/pull/13261#issuecomment-1433898716
Hi @bachmanity1! This is a bit outside my area of expertise and it's probably best for another committer to review.
--
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] bachmanity1 commented on pull request #13261: MINOR: after reading BYTES type it's possible to access data beyond its size
Posted by "bachmanity1 (via GitHub)" <gi...@apache.org>.
bachmanity1 commented on PR #13261:
URL: https://github.com/apache/kafka/pull/13261#issuecomment-1433923878
@ijuma could you please review this? 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] bachmanity1 commented on pull request #13261: MINOR: after reading BYTES type it's possible to access data beyond its size
Posted by "bachmanity1 (via GitHub)" <gi...@apache.org>.
bachmanity1 commented on PR #13261:
URL: https://github.com/apache/kafka/pull/13261#issuecomment-1437758582
@cmccabe could you please review this? 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