Skip to content
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 un-initialized JNI ClientInfo members #1161

Merged
merged 18 commits into from
Apr 30, 2024
Merged

Conversation

stefankiesz
Copy link
Contributor

@stefankiesz stefankiesz commented Apr 2, 2024

What was changed?

The previously un-initialized pClientInfo members in the JNI are now initialized to NULL values.

Why was it changed?

The kvsRetryStrategyCallbacks function pointers not being initialized as NULL was causing the function pointers to not be assigned in the PIC layer which was then causing a segmentation fault in the Producer C layer when running the Java Producer SDK.

Here in PIC, at least one of the pKvsRetryStrategyCallbacks function pointers must be NULL in order for setupDefaultKvsRetryStrategyParameters to be called to setup the default callbacks,

if (pKvsRetryStrategyCallbacks->createRetryStrategyFn == NULL || pKvsRetryStrategyCallbacks->freeRetryStrategyFn == NULL ||
pKvsRetryStrategyCallbacks->executeRetryStrategyFn == NULL || pKvsRetryStrategyCallbacks->getCurrentRetryAttemptNumberFn == NULL) {
        CHK_STATUS(setupDefaultKvsRetryStrategyParameters(pKinesisVideoClient));
}

but the function pointers were never explicitly initialized as NULL in the JNI, so they would usually all be junk, non-null values,

[testing] createRetryStrategyFn: 0xb33300b20100b3
[testing] freeRetryStrategyFn: 0x180062000000
[testing] executeRetryStrategyFn: 0x22001c0011001b00
[testing] getCurrentRetryAttemptNumberFn: 0x960001000000b174
#
# A fatal error has been detected by the Java Runtime Environment:
#
#  SIGBUS (0xa) at pc=0x00b33300b20100b3, pid=17942, tid=10243

causing the seg-fault to occur when the undefined createRetryStrategyFn is invoked in the below Producer C code,

CHK_STATUS(pKvsRetryStrategyCallbacks->createRetryStrategyFn(&(pKinesisVideoClient->deviceInfo.clientInfo.kvsRetryStrategy)));

and when the app did work, all the pointers happened to be NULL:

[testing] createRetryStrategyFn: 0x0
[testing] freeRetryStrategyFn: 0x0
[testing] executeRetryStrategyFn: 0x0
[testing] getCurrentRetryAttemptNumberFn: 0x0
[testing] Calling setupDefaultKvsRetryStrategyParameters

How was it changed?

Carefully, and by initializing previously un-initialized pClientInfo members in the JNI to NULL values.

What testing was done for the changes?

The Java sample successfully ran ~10 times using the outputted Producer CPP JNI with no issues. Previously, the application would seg-fault every ~5/6 runs.



By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@stefankiesz
Copy link
Contributor Author

Note to self: Testing shows JNI is setting the retry callbacks to null at this point, confirmed by seeing all four Couldn't find method id <type>RetryStrategyFn, setting to NULL logs

Copy link
Contributor

@niyatim23 niyatim23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refer this to fix the Windows build

@stefankiesz
Copy link
Contributor Author

Update from offline discussion:

  • The un-initialized pClientInfo members are now null-initialized using a memset(0) upon instantiation of the struct.
  • The KvsRetryStrategyCallbacks are not touched yet in the C-JNI-end. These will need to be setup in the KinesisVideoClientWrapper when we add this functionality to the Java SDK.
  • The passing of RetryStrategyType from Java to here in the JNI was successfully tested using logs.
  • In the future, we should consider fatally erroring out when setClientInfo() fails (we currently just log a warning).

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 47 lines in your changes are missing coverage. Please review.

Project coverage is 16.43%. Comparing base (eeb392f) to head (1965958).
Report is 7 commits behind head on develop.

❗ Current head 1965958 differs from pull request most recent head 44065e3. Consider uploading reports for the commit 44065e3 to get more accurate results

Files Patch % Lines
...mazonaws/kinesis/video/producer/jni/Parameters.cpp 0.00% 46 Missing ⚠️
...s/video/producer/jni/KinesisVideoClientWrapper.cpp 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1161      +/-   ##
===========================================
- Coverage    16.48%   16.43%   -0.06%     
===========================================
  Files           50       50              
  Lines         7019     7066      +47     
===========================================
+ Hits          1157     1161       +4     
- Misses        5862     5905      +43     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@hassanctech hassanctech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@stefankiesz stefankiesz merged commit f3bd374 into develop Apr 30, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants