You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@tsfile.apache.org by "ColinLeeo (via GitHub)" <gi...@apache.org> on 2026/07/27 10:43:25 UTC
[PR] fix(cpp): fix memory leaks and null dereference crashes [tsfile]
ColinLeeo opened a new pull request, #879:
URL: https://github.com/apache/tsfile/pull/879
## Summary
Fix 4 classes of memory/crash bugs discovered during a systematic scan of the C++ codebase.
## Changes
### 1. ChunkReader double-free (`src/reader/chunk_reader.cc`)
`reset()` frees the wrapped buffer via `mem_free(file_data_buf)` but does not clear the wrapped pointer in `ByteStream`. When `destroy()` is subsequently called, `get_wrapped_buf()` returns the already-freed pointer and `mem_free(buf)` is called again → **double-free crash**.
**Fix**: Call `in_stream_.clear_wrapped_buf()` immediately after `mem_free()` to null out the stale pointer. `destroy()` already has its own null-guard, so this prevents the double-free.
### 2. ChunkReader read-failure memory leak (`src/reader/chunk_reader.cc`)
`load_by_meta()` allocates `file_data_buf` via `mem_alloc()`, then calls `read_file_->read()`. The original code only freed the buffer on the partial-success path (`IS_SUCC(ret) && ret_read_len < MIN_SERIALIZED_SIZE`). If `read()` itself returns an error, the buffer is never freed and never passed to `wrap_from()` → **memory leak** for every failed chunk load.
**Fix**: Add an early-exit path after `read()`: if `!IS_SUCC(ret)`, free the buffer and return immediately.
### 3. LZ4Compressor error-path leaks (`src/compress/lz4_compressor.cc`)
`compress()` allocates `compressed_buf_` via `mem_alloc()`, then has two error paths that do not free it:
- `LZ4_compress_default` returns ≤ 0 → `compressed_buf_` leaked
- `mem_realloc` returns nullptr → `compressed_buf_` leaked
**Fix**: Add `mem_free(compressed_buf_); compressed_buf_ = nullptr;` on both error paths.
### 4. C wrapper string getter null dereference (`src/cwrapper/tsfile_cwrapper.cc`)
Both `tsfile_result_set_get_value_by_{name,index}_string()` call `r->get_value<common::String*>()` which delegates to `Field::get_string_value()`. For non-STRING/TEXT/BLOB column types, this returns `nullptr`. The code then unconditionally dereferences `ret->len_` → **SIGSEGV**.
**Fix**: Add a null check before the dereference, returning `nullptr` (which the Python caller already handles correctly, returning `None`).
## Verification
- All 721 C++ tests pass (`TsFile_Test` with ASan/Debug)
- clang-format applied via `spotless:apply`
## Files Changed
| File | +/− |
|------|-----|
| `cpp/src/reader/chunk_reader.cc` | +7/−1 |
| `cpp/src/compress/lz4_compressor.cc` | +4/−0 |
| `cpp/src/cwrapper/tsfile_cwrapper.cc` | +6/−0 |
--
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: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cpp): fix memory leaks and null dereference crashes [tsfile]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #879:
URL: https://github.com/apache/tsfile/pull/879#issuecomment-5090547790
## [Codecov](https://app.codecov.io/gh/apache/tsfile/pull/879?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `10 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 60.47%. Comparing base ([`a2fdf71`](https://app.codecov.io/gh/apache/tsfile/commit/a2fdf71161790af959a4311f49a88ff0c46abc4a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`b3a0002`](https://app.codecov.io/gh/apache/tsfile/commit/b3a0002f3712c6fdef7db452db3d2433a2ddb70b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/tsfile/pull/879?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [cpp/src/compress/lz4\_compressor.cc](https://app.codecov.io/gh/apache/tsfile/pull/879?src=pr&el=tree&filepath=cpp%2Fsrc%2Fcompress%2Flz4_compressor.cc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y3BwL3NyYy9jb21wcmVzcy9sejRfY29tcHJlc3Nvci5jYw==) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/tsfile/pull/879?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [cpp/src/reader/chunk\_reader.cc](https://app.codecov.io/gh/apache/tsfile/pull/879?src=pr&el=tree&filepath=cpp%2Fsrc%2Freader%2Fchunk_reader.cc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y3BwL3NyYy9yZWFkZXIvY2h1bmtfcmVhZGVyLmNj) | 0.00% | [2 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/tsfile/pull/879?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [cpp/src/cwrapper/tsfile\_cwrapper.cc](https://app.codecov.io/gh/apache/tsfile/pull/879?src=pr&el=tree&filepath=cpp%2Fsrc%2Fcwrapper%2Ftsfile_cwrapper.cc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y3BwL3NyYy9jd3JhcHBlci90c2ZpbGVfY3dyYXBwZXIuY2M=) | 0.00% | [0 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/tsfile/pull/879?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## develop #879 +/- ##
===========================================
- Coverage 60.48% 60.47% -0.02%
===========================================
Files 744 744
Lines 49014 49021 +7
Branches 7784 7787 +3
===========================================
- Hits 29646 29643 -3
- Misses 17966 17972 +6
- Partials 1402 1406 +4
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/tsfile/pull/879?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>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</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: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(cpp): fix memory leaks and null dereference crashes [tsfile]
Posted by "ColinLeeo (via GitHub)" <gi...@apache.org>.
ColinLeeo merged PR #879:
URL: https://github.com/apache/tsfile/pull/879
--
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: notifications-unsubscribe@tsfile.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org