You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "dkranchii (via GitHub)" <gi...@apache.org> on 2026/05/13 05:00:39 UTC
[PR] Fix InputStream leak in RenewableTlsUtils.createSSLFactory [pinot]
dkranchii opened a new pull request, #18483:
URL: https://github.com/apache/pinot/pull/18483
## What
Switch the key store and trust store `InputStream`s in `RenewableTlsUtils.createSSLFactory` to try-with-resources.
## Why
The streams were closed only on the success path, after `SSLFactory.builder().build()` returned. Any exception thrown in between — null trust-store password, malformed keystore, failure in `nl.altindag.ssl`'s builder, etc. — was caught and rewrapped as `IllegalStateException`, but the open stream was never closed. The leak is most likely to trigger during cert rotation (`reloadSslFactory` retries 3x and the file watcher catches mid-write states), which compounds it on long-running broker/server/controller/minion processes.
`TlsUtils.createKeyManagerFactory` and `TlsUtils.createTrustManagerFactory` in the same package already use this idiom, so the change also removes inconsistency.
## Safety
- `withIdentityMaterial(InputStream, char[], String)` and `withTrustMaterial(InputStream, char[], String)` consume the stream synchronously into a `KeyStore`; the stream is not retained for `build()`. Closing right after the `with...` call is equivalent to the prior post-`build()` close on the success path.
- No public API, wire-format, or config change.
- Exception type on failure is unchanged (`IllegalStateException` wrapping the cause).
## Test plan
- [ ] `./mvnw -pl pinot-common -am -Dtest=RenewableTlsUtilsTest -Dsurefire.failIfNoSpecifiedTests=false test`
- [ ] `./mvnw spotless:apply -pl pinot-common`
- [ ] `./mvnw checkstyle:check -pl pinot-common`
- [ ] `./mvnw license:check -pl pinot-common`
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix InputStream leak in RenewableTlsUtils.createSSLFactory [pinot]
Posted by "dkranchii (via GitHub)" <gi...@apache.org>.
dkranchii commented on PR #18483:
URL: https://github.com/apache/pinot/pull/18483#issuecomment-4561536480
@xiangfu0 can you please 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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix InputStream leak in RenewableTlsUtils.createSSLFactory [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #18483:
URL: https://github.com/apache/pinot/pull/18483
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix InputStream leak in RenewableTlsUtils.createSSLFactory [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #18483:
URL: https://github.com/apache/pinot/pull/18483#issuecomment-4437769272
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/18483?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 55.75%. Comparing base ([`d88887e`](https://app.codecov.io/gh/apache/pinot/commit/d88887ed57da45fd21d979e8a9ce99738cf6d5a1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`c2b9144`](https://app.codecov.io/gh/apache/pinot/commit/c2b91440f8c4808f83c8cfd58cab1dfaa6c03ceb?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 4 commits behind head on master.
> :exclamation: There is a different number of reports uploaded between BASE (d88887e) and HEAD (c2b9144). Click for more details.
>
> <details><summary>HEAD has 16 uploads less than BASE</summary>
>
>| Flag | BASE (d88887e) | HEAD (c2b9144) |
>|------|------|------|
>|java-21|5|1|
>|unittests|2|1|
>|temurin|5|1|
>|unittests2|1|0|
>|integration|3|0|
>|integration1|1|0|
>|integration2|1|0|
>|custom-integration1|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #18483 +/- ##
============================================
- Coverage 63.68% 55.75% -7.94%
+ Complexity 1684 816 -868
============================================
Files 3262 2554 -708
Lines 199835 148230 -51605
Branches 31034 23945 -7089
============================================
- Hits 127266 82645 -44621
+ Misses 62416 58517 -3899
+ Partials 10153 7068 -3085
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/18483/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/18483/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/18483/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/18483/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/18483/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/18483/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.75% <100.00%> (-7.94%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/18483/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.75% <100.00%> (-7.94%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/18483/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.75% <100.00%> (-7.93%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/18483/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.75% <100.00%> (-0.03%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/18483/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/18483?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix InputStream leak in RenewableTlsUtils.createSSLFactory [pinot]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on PR #18483:
URL: https://github.com/apache/pinot/pull/18483#issuecomment-4966078425
The stream lifetime here checks out: `withIdentityMaterial(InputStream, ...)` and `withTrustMaterial(InputStream, ...)` read the stream into a `KeyStore` right at the call, so closing inside the tightened `try` block instead of after `build()` loses nothing on the success path, and it actually closes the fd on the paths where the trust store `openStream()` or `build()` used to throw before the old close ran. Nice that it lines up with the try-with-resources `TlsUtils.createKeyManagerFactory` already uses a few methods over in the same package. Happy to approve.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Fix InputStream leak in RenewableTlsUtils.createSSLFactory [pinot]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on PR #18483:
URL: https://github.com/apache/pinot/pull/18483#issuecomment-4912317188
Verified against the diff - correct and low-risk. nl.altindag.ssl's withIdentityMaterial(InputStream, char[], String) and withTrustMaterial(...) read the stream into a KeyStore synchronously and don't retain it, so scoping each stream in a try-with-resources and closing it right after the with*Material call is equivalent to the old post-build() close on the success path. The win is on the failure path: each stream is now closed before the next potentially-throwing step, so the key-store stream no longer leaks when a later step (the trust-store null-password Precondition, malformed trust store, or builder error in build()) throws - which is the actual FD leak. Swappable ordering is preserved (withSwappable*Material still precedes the corresponding with*Material), and moving openStream() after the swappable call is inert since opening the stream doesn't touch the builder. Matches the try-with-resources idiom already used by TlsUtils.createKeyManagerFactory and createTrustManagerFact
ory in the same package.
One coverage note: RenewableTlsUtilsTest's exception tests exercise the enableAutoRenewal ftp:// validation path, not createSSLFactory's open-then-fail path, so the specific leak being fixed isn't directly asserted. Hard to assert cleanly without spying on openStream(), so a minor gap rather than a blocker.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org