fix: harden core C library for security, correctness, and code quality#1302
fix: harden core C library for security, correctness, and code quality#1302scholarsmate merged 15 commits intomainfrom
Conversation
Security and correctness fixes: - Fix snprintf buffer overflow in edit.cpp (FILENAME_MAX -> FILENAME_MAX + 1) - Convert recursive undo/redo to iterative loops to prevent stack overflow - Add thread_local to static buffers in filesystem.cpp for thread safety - Add odd-length input validation to omega_encode_hex2bin - Guard DEBUG define with #ifndef to support conditional compilation - Remove unsafe mkstemp assert in filesystem.cpp Design and code quality fixes: - Replace assert() with proper error returns in all public API functions across segment, change, search, viewport, session, check, utility, and filesystem modules - Replace magic number 7 with sizeof(omega_data_t) - 1 in segment.cpp - Fix DBG macro parenthesization in macros.h - Rename stl_string_adapter.cpp to stl_string_adaptor.cpp to match header - Add negative capacity guard in segment creation Test coverage: - Add Hex2Bin Odd Length test (7 assertions) - Add Large Transaction Undo/Redo test (114 assertions) - Add Null Pointer Safety test for core APIs (53 assertions) - Add Null Pointer Safety test for utility APIs (14 assertions) - Add Null Pointer Safety test for filesystem APIs (11 assertions) - Add Segment Small Data Optimization test (11 assertions)
There was a problem hiding this comment.
Pull request overview
This PR hardens the Ωedit core library’s C/C++ APIs against common correctness and security issues (null-pointer handling, bounds checks, stack safety, thread safety), and expands the core test suite to validate these behaviors.
Changes:
- Replace a number of
assert()-based preconditions in public APIs with defensive checks + error returns. - Convert undo/redo transaction processing to iterative loops to avoid recursion-driven stack overflows.
- Add targeted tests for odd-length hex decoding, large transaction undo/redo, segment SDO behavior, and null-pointer safety across multiple modules.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| core/src/lib/edit.cpp | Adjusts temp/checkpoint filename buffers; converts undo/redo to iterative loops. |
| core/src/lib/filesystem.cpp | Adds thread_local buffers and replaces some asserts with runtime validation. |
| core/src/lib/search.cpp | Adds runtime input validation for search context creation and getters. |
| core/src/lib/segment.cpp | Adds negative-capacity guard and replaces magic threshold with sizeof(omega_data_t) - 1. |
| core/src/lib/session.cpp | Replaces many assert(session_ptr) preconditions with null-safe return values. |
| core/src/lib/viewport.cpp | Makes viewport getters/setters null-safe (returning defaults). |
| core/src/lib/utility.c | Adds null/arg validation and improved error handling for utility helpers. |
| core/src/lib/encode.c | Adds odd-length hex input validation. |
| core/src/lib/impl_/macros.h | Fixes DBG(x) macro parenthesization. |
| core/src/lib/check.cpp | Makes omega_check_model null-safe. |
| core/src/lib/change.cpp | Makes change getters null-safe. |
| core/src/lib/stl_string_adaptor.cpp | Introduces/renames STL string adaptor implementation. |
| core/src/include/omega_edit/config.h | Stops unconditionally defining DEBUG in the public header. |
| core/CMakeLists.txt | Defines DEBUG in Debug builds via CMake compile definitions. |
| core/src/tests/session_tests.cpp | Adds large-transaction undo/redo test and broad null-safety test coverage. |
| core/src/tests/omegaEdit_tests.cpp | Adds segment small-data-optimization tests (incl. negative capacity). |
| core/src/tests/utility_tests.cpp | Adds utility null-pointer safety tests. |
| core/src/tests/filesystem_tests.cpp | Adds filesystem null-pointer safety tests. |
| core/src/tests/encode_tests.cpp | Adds odd-length hex2bin tests. |
| size_t omega_encode_hex2bin(const char *src, omega_byte_t *dst, size_t src_length) { | ||
| assert(src); | ||
| assert(dst); | ||
| if (src_length & 1) { | ||
| /* Odd-length hex string is invalid */ | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
omega_encode_hex2bin() still relies on assert(src) / assert(dst) and writes a null terminator to dst (dst[i] = '\0'), but encode.h documents dst as needing only (src_length / 2) bytes. This combination can lead to out-of-bounds writes or null dereferences in release builds; either stop writing the terminator or update the API contract and add runtime null checks that return 0 on invalid inputs.
core/src/lib/search.cpp
Outdated
| if (!session_ptr || !pattern || session_offset < 0) { return nullptr; } | ||
| pattern_length = | ||
| pattern_length ? pattern_length : static_cast<int64_t>(strlen(reinterpret_cast<const char *>(pattern))); | ||
| assert(0 < pattern_length); |
There was a problem hiding this comment.
omega_search_create_context_bytes() still has an assert(0 < pattern_length) after deriving pattern_length via strlen(). With an empty pattern this will abort in Debug builds (and may behave unexpectedly in Release); consider returning nullptr when pattern_length <= 0 instead of asserting, consistent with the other new input validation in this function.
| assert(0 < pattern_length); | |
| if (pattern_length <= 0) { return nullptr; } |
| int64_t omega_edit_undo_last_change(omega_session_t *session_ptr) { | ||
| if ((omega_session_changes_paused(session_ptr) == 0) && !session_ptr->models_.back()->changes.empty()) { | ||
| int64_t result = 0; | ||
| while ((omega_session_changes_paused(session_ptr) == 0) && !session_ptr->models_.back()->changes.empty()) { | ||
| const auto change_ptr = session_ptr->models_.back()->changes.back(); | ||
| session_ptr->models_.back()->changes.pop_back(); |
There was a problem hiding this comment.
omega_edit_undo_last_change() now dereferences session_ptr->models_ inside the loop without validating session_ptr first. Because omega_session_changes_paused(nullptr) returns 0, passing a null session can enter the loop and crash; add an early guard (e.g., return 0 or -1) before accessing session_ptr internals.
| if ((omega_session_changes_paused(session_ptr) == 0) && !session_ptr->models_.back()->changes_undone.empty()) { | ||
| while ((omega_session_changes_paused(session_ptr) == 0) && !session_ptr->models_.back()->changes_undone.empty()) { | ||
| const auto change_ptr = session_ptr->models_.back()->changes_undone.back(); | ||
| rc = update_(session_ptr, change_ptr); |
There was a problem hiding this comment.
omega_edit_redo_last_undo() continues looping even if update_() fails (rc < 0), potentially partially redoing a transaction after an error. Consider breaking/returning immediately on rc < 0 so callers get a clear failure signal and the session isn't advanced further in an error state.
| rc = update_(session_ptr, change_ptr); | |
| rc = update_(session_ptr, change_ptr); | |
| if (rc < 0) { | |
| // On failure, stop redoing and return the error without modifying the undo stack further | |
| return rc; | |
| } |
| char *omega_util_dirname(char const *path, char *buffer) { | ||
| assert(path); | ||
| static char buff[FILENAME_MAX]{};//create string buffer to hold path | ||
| if (!path || !*path) { return nullptr; } | ||
| static thread_local char buff[FILENAME_MAX]{}; | ||
| if (!buffer) { buffer = buff; } | ||
| auto const dirname_str = fs::path(path).parent_path().string(); | ||
| assert(0 <= dirname_str.length()); | ||
| assert(FILENAME_MAX > dirname_str.length()); | ||
| if (dirname_str.length() >= FILENAME_MAX) { return nullptr; } | ||
| auto const len = dirname_str.copy(buffer, dirname_str.length()); |
There was a problem hiding this comment.
omega_util_dirname() can now return nullptr for valid-but-too-long paths. Callers in this file (e.g., omega_util_available_filename builds std::string from omega_util_dirname/omega_util_basename results) assume a non-null C-string, so this change can turn an oversize path into UB/crashes. Update those callers to handle nullptr returns (or provide a safe non-null fallback string) to match the new contract.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@scholarsmate I've opened a new pull request, #1303, to work on those changes. Once the pull request is ready, I'll request review from you. |
core/src/lib/utility.c
Outdated
| int omega_util_strnicmp(const char *s1, const char *s2, uint64_t sz) { | ||
| if (!s1 || !s2) { return s1 == s2 ? 0 : (s1 ? 1 : -1); } | ||
| int rc = 0; | ||
| for (uint64_t i = 0; i < sz; ++i) { if (0 != (rc = tolower(s1[i]) - tolower(s2[i]))) break; } |
There was a problem hiding this comment.
tolower() is undefined for negative char values (common when char is signed and bytes are >= 0x80). Cast to unsigned char before calling tolower (and similarly for any other ctype.h calls) to avoid UB when comparing non-ASCII data.
| for (uint64_t i = 0; i < sz; ++i) { if (0 != (rc = tolower(s1[i]) - tolower(s2[i]))) break; } | |
| for (uint64_t i = 0; i < sz; ++i) { | |
| if (0 != (rc = tolower((unsigned char) s1[i]) - tolower((unsigned char) s2[i]))) break; | |
| } |
core/src/lib/edit.cpp
Outdated
| if (FILENAME_MAX <= snprintf(checkpoint_filename, FILENAME_MAX, "%s%c.OmegaEdit-chk.%zu.XXXXXX", | ||
| checkpoint_directory, omega_util_directory_separator(), session_ptr->models_.size())) { |
There was a problem hiding this comment.
The buffer was increased to FILENAME_MAX + 1, but snprintf still uses FILENAME_MAX as the size argument and comparison threshold. Use the actual buffer size (e.g., sizeof(checkpoint_filename)) for both the snprintf size parameter and truncation check so the code is self-consistent and won’t regress if the buffer size changes again.
| if (FILENAME_MAX <= snprintf(checkpoint_filename, FILENAME_MAX, "%s%c.OmegaEdit-chk.%zu.XXXXXX", | |
| checkpoint_directory, omega_util_directory_separator(), session_ptr->models_.size())) { | |
| const auto snprintf_result = snprintf(checkpoint_filename, sizeof(checkpoint_filename), | |
| "%s%c.OmegaEdit-chk.%zu.XXXXXX", checkpoint_directory, | |
| omega_util_directory_separator(), session_ptr->models_.size()); | |
| if (snprintf_result < 0 || static_cast<size_t>(snprintf_result) >= sizeof(checkpoint_filename)) { |
| /** Define to enable debugging */ | ||
| #define DEBUG | ||
| #ifndef DEBUG | ||
| /* DEBUG should be set by the build system (e.g., -DDEBUG in CMake Debug builds) */ | ||
| #endif |
There was a problem hiding this comment.
This comment block is now misleading: it says “Define to enable debugging” but the header no longer defines DEBUG, and the #ifndef DEBUG block has no effect other than a comment. Consider rewriting this section to clearly state that DEBUG is not defined by the public header and must be provided by the build system (or remove the empty conditional entirely).
* Initial plan * fix: address review comments - null checks, error handling, nullptr guards Co-authored-by: scholarsmate <2205472+scholarsmate@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: scholarsmate <2205472+scholarsmate@users.noreply.github.com>
| return -1; | ||
| } | ||
| char temp_filename[FILENAME_MAX]; | ||
| char temp_filename[FILENAME_MAX + 1]; |
There was a problem hiding this comment.
temp_filename was expanded to FILENAME_MAX + 1, but later in this function snprintf still uses FILENAME_MAX as its size argument and the truncation check compares against FILENAME_MAX. That combination can still incorrectly treat a just-fitting path as truncated (and doesn’t actually take advantage of the extra byte). Use sizeof(temp_filename) consistently for snprintf and truncation checks, and ensure temp_filename is initialized/validated before any reads in case omega_util_dirname()/omega_util_get_current_dir() fail and leave it unwritten.
| char temp_filename[FILENAME_MAX + 1]; | |
| char temp_filename[FILENAME_MAX]; | |
| temp_filename[0] = '\0'; |
| char checkpoint_filename[FILENAME_MAX + 1]; | ||
| if (FILENAME_MAX <= snprintf(checkpoint_filename, FILENAME_MAX, "%s%c.OmegaEdit-chk.%zu.XXXXXX", | ||
| checkpoint_directory, omega_util_directory_separator(), session_ptr->models_.size())) { | ||
| LOG_ERROR("failed to create checkpoint filename template"); |
There was a problem hiding this comment.
checkpoint_filename is FILENAME_MAX + 1, but snprintf is called with FILENAME_MAX and the truncation check uses FILENAME_MAX. This can spuriously fail for templates that would fit in the allocated buffer. Pass sizeof(checkpoint_filename) to snprintf and update the truncation check accordingly.
607236f to
6b7d96d
Compare
6b7d96d to
5fb8a75
Compare
…pe assertions in coverageGaps.spec.ts
Some client functions reject with string errors (getComputedFileSize, destroySession) while others reject with Error objects (insert, del). Use expect(err).to.exist to accept both types.
core/src/lib/filesystem.cpp
Outdated
| auto const absolute_path_str = fs::absolute(fs::canonical(path)).string(); | ||
| assert(0 < absolute_path_str.length()); | ||
| assert(FILENAME_MAX > absolute_path_str.length()); | ||
| if (absolute_path_str.empty() || absolute_path_str.length() >= FILENAME_MAX) { return nullptr; } | ||
| auto const len = absolute_path_str.copy(buffer, absolute_path_str.length()); |
There was a problem hiding this comment.
omega_util_normalize_path() calls std::filesystem::canonical() / absolute() which can throw (nonexistent path, permission issues). Wrap this in a try/catch for std::filesystem::filesystem_error and return nullptr (optionally log) instead of letting an exception terminate the process.
| const session1 = await createSession() | ||
| const session1_id = session1.getSessionId() | ||
| const session2 = await createSession() | ||
| const session2_id = session2.getSessionId() | ||
| expect(await getSessionCount()).to.equal(2) |
There was a problem hiding this comment.
This test assumes the global session count is exactly 2 after creating two sessions. If any other spec leaks a session (or a previous test in this file fails before cleanup), this becomes flaky. Consider asserting the initial count is 0 (or checking that the count increased by 2) before the expect(...).to.equal(2) assertion.
core/src/lib/edit.cpp
Outdated
| assert(session_ptr); | ||
| assert(file_path); | ||
| assert(0 <= offset); | ||
| if (!session_ptr || !file_path || offset < 0) { return -1; } |
There was a problem hiding this comment.
omega_edit_save_segment() checks file_path != nullptr but not that it’s non-empty. Downstream calls (e.g., omega_util_dirname) now treat empty paths as invalid and may return nullptr, which can lead to undefined behavior. Add a !*file_path guard up front (and consider validating saved_file_path buffer expectations too).
| if (!session_ptr || !file_path || offset < 0) { return -1; } | |
| if (!session_ptr || !file_path || !*file_path || offset < 0) { return -1; } |
| int omega_visit_changes(const omega_session_t *session_ptr, omega_session_change_visitor_cbk_t cbk, void *user_data) { | ||
| assert(session_ptr); | ||
| if (!session_ptr) { return -1; } | ||
| int rc = 0; | ||
| for (const auto &iter: session_ptr->models_.back()->changes) { | ||
| if ((rc = cbk(iter.get(), user_data)) != 0) { break; } |
There was a problem hiding this comment.
omega_visit_changes*() now checks session_ptr but still unconditionally calls cbk(...). If a caller passes a null visitor callback, this will segfault. Add a if (!cbk) return -1; (or 0, depending on the API contract) similar to the session_ptr guard.
| LOG_ERROR("failed to create checkpoint filename template"); | ||
| return -1; | ||
| } | ||
| const auto checkpoint_fd = omega_util_mkstemp(checkpoint_filename, 0600);// S_IRUSR | S_IWUSR |
There was a problem hiding this comment.
omega_util_mkstemp() return value isn’t checked before close(). If mkstemp fails, the filename template still contains XXXXXX and the code will continue, potentially creating/writing a non-unique checkpoint path. Check checkpoint_fd < 0 and return an error (only close when checkpoint_fd >= 0).
| const auto checkpoint_fd = omega_util_mkstemp(checkpoint_filename, 0600);// S_IRUSR | S_IWUSR | |
| const auto checkpoint_fd = omega_util_mkstemp(checkpoint_filename, 0600);// S_IRUSR | S_IWUSR | |
| if (checkpoint_fd < 0) { | |
| LOG_ERROR("omega_util_mkstemp failed for checkpoint file '" << checkpoint_filename << "'"); | |
| return -1; | |
| } |
| const auto *const checkpoint_directory = omega_session_get_checkpoint_directory(session_ptr); | ||
| // make sure the checkpoint directory exists | ||
| if (omega_util_directory_exists(checkpoint_directory) == 0) { | ||
| LOG_ERROR("checkpoint directory '" << checkpoint_directory << "' does not exist"); |
There was a problem hiding this comment.
If the checkpoint directory doesn’t exist, omega_edit_create_checkpoint() logs an error but continues. This likely leads to mkstemp/open failures later; it should return -1 immediately when omega_util_directory_exists() is false.
| LOG_ERROR("checkpoint directory '" << checkpoint_directory << "' does not exist"); | |
| LOG_ERROR("checkpoint directory '" << checkpoint_directory << "' does not exist"); | |
| return -1; |
| char checkpoint_filename[FILENAME_MAX + 1]; | ||
| if (FILENAME_MAX <= snprintf(checkpoint_filename, FILENAME_MAX, "%s%c.OmegaEdit-chk.%zu.XXXXXX", | ||
| checkpoint_directory, omega_util_directory_separator(), session_ptr->models_.size())) { | ||
| LOG_ERROR("failed to create checkpoint filename template"); |
There was a problem hiding this comment.
The checkpoint template creation is guarded by snprintf, but the subsequent omega_util_mkstemp(checkpoint_filename, ...) result must be checked for failure before calling close() and continuing; otherwise the XXXXXX template may remain unresolved and a non-unique/invalid checkpoint path can be used.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@scholarsmate I've opened a new pull request, #1304, to work on those changes. Once the pull request is ready, I'll request review from you. |
…s (round 4) (#1304) * Initial plan * fix: address review comments from PR 1302 review 3907448840 Co-authored-by: scholarsmate <2205472+scholarsmate@users.noreply.github.com> * fix: improve snprintf checks with explicit result variable and dual condition Co-authored-by: scholarsmate <2205472+scholarsmate@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: scholarsmate <2205472+scholarsmate@users.noreply.github.com>
Security and correctness fixes:
Design and code quality fixes:
Test coverage: