-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix buffer leak and jni bytes object leak for jni command/write #28913
Merged
yunhanw-google
merged 1 commit into
project-chip:master
from
yunhanw-google:bug/fix_command_leak
Aug 28, 2023
Merged
Fix buffer leak and jni bytes object leak for jni command/write #28913
yunhanw-google
merged 1 commit into
project-chip:master
from
yunhanw-google:bug/fix_command_leak
Aug 28, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
electrocucaracha,
gjc13,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo,
jtung-apple,
kkasperczyk-no,
ksperling-apple,
lazarkov,
lpbeliveau-silabs and
LuDuda
August 27, 2023 03:12
PR #28913: Size comparison from 2d5fe2c to 6eef237 Decreases (2 builds for efr32, telink)
Full report (62 builds for bl602, bl702, bl702l, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
andy31415
reviewed
Aug 28, 2023
andy31415
reviewed
Aug 28, 2023
andy31415
reviewed
Aug 28, 2023
andy31415
reviewed
Aug 28, 2023
yunhanw-google
force-pushed
the
bug/fix_command_leak
branch
2 times, most recently
from
August 28, 2023 15:12
2846b96
to
2981973
Compare
yunhanw-google
force-pushed
the
bug/fix_command_leak
branch
from
August 28, 2023 15:16
2981973
to
4f325a1
Compare
andy31415
reviewed
Aug 28, 2023
PR #28913: Size comparison from baa102e to 4f325a1 Full report (19 builds for bl602, bl702, bl702l, cc32xx, k32w, linux, mbed, nrfconnect, qpg)
|
andy31415
reviewed
Aug 28, 2023
andy31415
reviewed
Aug 28, 2023
andy31415
approved these changes
Aug 28, 2023
yunhanw-google
force-pushed
the
bug/fix_command_leak
branch
from
August 28, 2023 16:07
4f325a1
to
77c3f60
Compare
yunhanw-google
force-pushed
the
bug/fix_command_leak
branch
from
August 28, 2023 16:10
77c3f60
to
191082e
Compare
saurabhst
approved these changes
Aug 28, 2023
PR #28913: Size comparison from b9b2ead to 191082e Decreases (2 builds for bl702l, efr32)
Full report (62 builds for bl602, bl702, bl702l, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
HunsupJung
pushed a commit
to HunsupJung/connectedhomeip
that referenced
this pull request
Oct 23, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
-- Use RAII to fix the existing jni bug where jni bytes object has not yet release for im command and write when processing tlv bytes
-- Refactor command and write code to ConvertJsonToTlvWithoutStruct function in terms of json conversion where ConvertJsonToTlvWithoutStruct
-- Fix buffer leaks for invoke and write
-- For IM invoke json processing, since the invoke in sdk does not support chunk, kMaxSecureSduLengthBytes should be enough for command json blob
-- For IM write, since our write in sdk supports the chunked capability, for example, oversized list could be chunked in multiple message. When transforming JSON to TLV, we need know the actual size for tlv blob when handling JsonToTlv.
We allocate memory using json string's size, which is large enough to hold the corresponding tlv blob.
The better way is to implement memory auto-grow to get the actual size needed for tlv blob when transforming tlv to json.
This is current jsontlv c++ lib's limitation. In contrast, kotlin jsontlv use Java's ByteArrayOutputStream which can auto-grow when memory is not enough.
-- Optimize code and improve code readability.
-- Testing: Existing test cover the functionality, manual testing to check leaks