Fix core stability, correctness, and memory issues #1261
Conversation
- Fix infinite loop and memory leak in Windows network tools (networkTools.cpp) - Improve exception safety in removeAccents using RAII (stringTools.cpp) - Change getBookById to return by value to avoid dangling references (library.cpp) - Use proper constant for HTTP 416 instead of magic number (response.cpp) - Fix exception slicing in getDownload re-throw (downloader.cpp) Signed-off-by: shbhmexe <[email protected]>
|
@shbhmexe Thank you for your various improvement proposals. When you are ready, we will assign a technical reviewer for your code. But before we come to that, please split your commit in smaller pieces with one commit per "logical" change. That way we can clearly understand and follow the changes, one by one. |
@kelson42 Thank you for the feedback. I am ready to proceed. I will split the changes into separate logical commits as requested to make the review process easier. I'll adhere to this practice for future contributions as well. |
There was a problem hiding this comment.
Pull request overview
This PR addresses multiple critical correctness, stability, and memory management issues across different components of libkiwix:
- Fixed a critical infinite loop and memory leak in Windows network interface enumeration
- Improved thread-safety by changing
Library::getBookByIdandLibrary::getBookByPathto return by value instead of const reference - Fixed exception type preservation in the downloader component
- Enhanced exception safety using RAII for ICU resource management
- Replaced a magic number with the appropriate HTTP constant
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/tools/networkTools.cpp | Fixes infinite loop by incrementing Iterations counter and prevents memory leak by freeing previous allocation before retry |
| include/library.h | Changes getBookById and getBookByPath signatures to return by value for thread-safety |
| src/library.cpp | Implements return-by-value changes and updates all internal usages; also adds explicit std:: namespace qualification for stable_sort |
| src/name_mapper.cpp | Updates to use value semantics when retrieving books, consistent with new API |
| src/tools/stringTools.cpp | Replaces manual memory management with std::unique_ptr for ICU transliterator, improving exception safety |
| src/downloader.cpp | Changes from throw e to throw to preserve original exception type and avoid slicing |
| src/server/response.cpp | Replaces magic number 416 with MHD_HTTP_RANGE_NOT_SATISFIABLE constant for better code clarity |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1261 +/- ##
==========================================
- Coverage 42.87% 42.86% -0.01%
==========================================
Files 60 60
Lines 4744 4745 +1
Branches 2498 2505 +7
==========================================
Hits 2034 2034
Misses 1088 1088
- Partials 1622 1623 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
These are bug fixes for existing code paths. They don't introduce new testable logic, so adding tests isn't applicable here. Could you override the codecov check? |
|
@shbhmexe Any news? |
@veloman-yunkan Thank you for the review. I understand the concern regarding ABI compatibility with getBookById and getBookByPath. I have reverted those changes in this PR as requested. I agree that the thread-safety issue warrants a separate discussion/ticket. This PR now only contains the other stability and correctness fixes. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Increment the iteration counter to prevent infinite loops when GetAdaptersAddresses returns ERROR_BUFFER_OVERFLOW. - Free the previously allocated buffer before re-allocating to prevent memory leaks on retry. Signed-off-by: shbhmexe <[email protected]>
veloman-yunkan
left a comment
veloman-yunkan
left a comment
There was a problem hiding this comment.
I am fine with the changes in this PR, but I don't like its commit structure/history. Please rewrite (via git rebase) the PR branch so that there is one commit per logical change. Also please update the PR description.
3 participants
This PR addresses several logical issues and potential bugs across different components of libkiwix:
getNetworkInterfacesWin. TheIterationscounter was never incremented, andmallocwas called in a loop without correspondingfreecalls on retry.Library::getBookByIdto return by value. Returning aconst referenceto a map entry is unsafe if the map is modified (e.g. during library refresh), leading to dangling references.Downloader::getDownload. The original exception type is now preserved during re-throw.stringTools.cppby using RAII (std::unique_ptr) for ICU transliterators.416with the appropriateMHD_HTTP_RANGE_NOT_SATISFIABLEconstant inresponse.cpp.These changes improve the overall reliability and correctness of the library without changing its public API or existing functionality.
Verification