Fix memory issues and add Address Sanitizer to Debug configuration. #4

asn5d · 2025-12-02T23:13:03Z

This PR fixes memory issues reported in #2 and #3 and adds address sanitizer library when building in Debug mode to catch similar bugs.

Bug fixes

Fix segmentation fault in bwthin caused by invalid memory access.
Fix integer overflow and potential out-of-bounds access in bwthin.
Fix additional memory issues detected while running with AddressSanitizer.

ASAN Debug configuration

Add CMake support to build cvutil in Debug mode with AddressSanitizer enabled.
Used this configuration to find and verify the fixes above.
This can be reused by contributors to validate future changes.

Fixes #2
Fixes #3

Copilot

Pull request overview

This PR addresses critical memory bugs and adds AddressSanitizer (ASAN) support to help detect similar issues in the future. The changes include fixing off-by-one errors in loop boundaries, correcting memory access bugs in the bwthin algorithm, and refactoring the CMake build system to use modern interface libraries for compiler flags with ASAN enabled in Debug mode.

Fixed off-by-one errors in SIMD loop boundaries across multiple functions
Corrected boundary condition checks in bwthin to prevent out-of-bounds array access
Added ASAN integration for MSVC, Clang, and GCC compilers in Debug configuration
Improved performance by passing large objects (vectors, strings) by const reference

Reviewed changes

Copilot reviewed 14 out of 15 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
CMakeLists.txt	Refactored compiler flags into interface library, added ASAN support for Debug builds, and updated compiler selection logic
cvutil/CMakeLists.txt	Added cvutil_compiler_flags linkage and commented out legacy build directory configuration
cvutil/cvutil_matlab_interface.cpp	Fixed off-by-one errors in SIMD loops and changed string parameter to const reference
cvutil/cvutil_matlab_interface.h	Updated function signature to pass string by const reference
cvutil/cvutil_core.cpp	Fixed incorrect matrix dimension usage when adding padding in bwthin
cvutil/cvutil_core.h	Updated doughlas_peucker signature to pass vector by const reference
cvutil/cvutil_linesim.cpp	Modified doughlas_peucker to use local copy when modifying input, preventing modification of const reference
cvutil/cvutil_linesim.h	Updated function signature to pass vector by const reference
cvutil/cvutil_bwthin.cpp	Added bounds checks before array accesses to prevent out-of-bounds access
cvutil/cvutil_bwskel.cpp	Corrected boundary calculations to account for 2-row padding instead of 1-row
cvutil/cvutil_bwdist.cpp	Fixed off-by-one error in loop boundary for unaligned elements
cvutilConfig.cmake.in	Reverted from install(FILES) back to file(INSTALL) for dependency copying
PluginManager/CMakeLists.txt	Added cvutil_compiler_flags linkage and commented out legacy configuration
RoiManager/CMakeLists.txt	Added cvutil_compiler_flags linkage and commented out legacy configuration
.gitignore	Added [Ii]nstall/ directory to ignore list

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-02T23:21:29Z

cvutil/cvutil_bwskel.cpp

+            if (dptr[ploc->getpt() - 1] > dptr[ploc->getpt()] && 
+                iptr[ploc->getpt()] > 0 && ploc->getpt() >= (2 * ncols) && 
+                ploc->getimageneighborsum() <= 2 && ploc->getimageconnectivity() < 2)
            {
                do
                {
                    ploc->unsetimagepixel();
                    ploc = ploc->left();
-                } while (dptr[ploc->getpt() - 1] > dptr[ploc->getpt()] && iptr[ploc->getpt()] > 0 && ploc->getpt() >= ncols && ploc->getimageneighborsum() <= 2 && ploc->getimageconnectivity() < 2);
+                } while (dptr[ploc->getpt() - 1] > dptr[ploc->getpt()] && 
+                         iptr[ploc->getpt()] > 0 && ploc->getpt() >= (2 * ncols) && 
+                         ploc->getimageneighborsum() <= 2 && ploc->getimageconnectivity() < 2);


Potential out-of-bounds access: dptr[ploc->getpt() - 1] is accessed without verifying that ploc->getpt() - 1 is non-negative. The loop condition checks ploc->getpt() >= (2 * ncols), but this doesn't prevent accessing ploc->getpt() - 1 which could be less than 0 when ploc->getpt() equals 0. Consider adding a bounds check before accessing dptr[ploc->getpt() - 1].

Copilot · 2025-12-02T23:21:29Z

cvutil/CMakeLists.txt

+# file(MAKE_DIRECTORY ${CMAKE_BINARY_DIR}/library/bin)
+# file(MAKE_DIRECTORY ${CMAKE_BINARY_DIR}/library/include)
+# file(MAKE_DIRECTORY ${CMAKE_BINARY_DIR}/library/lib)

-# Set the output directories for the shared library (bin for runtime, lib for static and import libraries)
-set_target_properties(cvutil PROPERTIES
-    RUNTIME_OUTPUT_DIRECTORY_DEBUG ${CMAKE_BINARY_DIR}/library/bin/
-    RUNTIME_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR}/library/bin/
+# # Set the output directories for the shared library (bin for runtime, lib for static and import libraries)
+# set_target_properties(cvutil PROPERTIES
+#     RUNTIME_OUTPUT_DIRECTORY_DEBUG ${CMAKE_BINARY_DIR}/library/bin/
+#     RUNTIME_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR}/library/bin/

-    LIBRARY_OUTPUT_DIRECTORY_DEBUG ${CMAKE_BINARY_DIR}/library/lib/
-    LIBRARY_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR}/library/lib/
+#     LIBRARY_OUTPUT_DIRECTORY_DEBUG ${CMAKE_BINARY_DIR}/library/lib/
+#     LIBRARY_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR}/library/lib/

-    ARCHIVE_OUTPUT_DIRECTORY_DEBUG ${CMAKE_BINARY_DIR}/library/lib/
-    ARCHIVE_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR}/library/lib/
-)
+#     ARCHIVE_OUTPUT_DIRECTORY_DEBUG ${CMAKE_BINARY_DIR}/library/lib/
+#     ARCHIVE_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR}/library/lib/
+# )

-# Post-build events headers to appropriate directory
-add_custom_command(TARGET cvutil POST_BUILD
-    COMMAND ${CMAKE_COMMAND} -E copy_if_different
-        ${CMAKE_CURRENT_SOURCE_DIR}/cvutil.h ${CMAKE_BINARY_DIR}/library/include/
-    COMMAND ${CMAKE_COMMAND} -E copy_if_different
-        ${CMAKE_CURRENT_SOURCE_DIR}/cvutil_core.h ${CMAKE_BINARY_DIR}/library/include/
-    COMMAND ${CMAKE_COMMAND} -E copy_if_different
-        ${CMAKE_CURRENT_SOURCE_DIR}/cvutil_matlab_interface.h ${CMAKE_BINARY_DIR}/library/include/
-    COMMAND ${CMAKE_COMMAND} -E copy_if_different
-        ${CMAKE_CURRENT_SOURCE_DIR}/cvutil_templates.h ${CMAKE_BINARY_DIR}/library/include/
-    COMMAND ${CMAKE_COMMAND} -E copy_if_different
-        ${CMAKE_CURRENT_SOURCE_DIR}/figure.h ${CMAKE_BINARY_DIR}/library/include/
-    COMMAND ${CMAKE_COMMAND} -E copy_if_different
-        ${CMAKE_CURRENT_SOURCE_DIR}/profiler.h ${CMAKE_BINARY_DIR}/library/include/
-    COMMAND ${CMAKE_COMMAND} -E copy_if_different
-        ${CMAKE_CURRENT_SOURCE_DIR}/stdproto.h ${CMAKE_BINARY_DIR}/library/include/
-    COMMAND ${CMAKE_COMMAND} -E copy_if_different
-        ${CMAKE_CURRENT_SOURCE_DIR}/video.h ${CMAKE_BINARY_DIR}/library/include/
-)
+# # Post-build events headers to appropriate directory
+# add_custom_command(TARGET cvutil POST_BUILD
+#     COMMAND ${CMAKE_COMMAND} -E copy_if_different
+#         ${CMAKE_CURRENT_SOURCE_DIR}/cvutil.h ${CMAKE_BINARY_DIR}/library/include/
+#     COMMAND ${CMAKE_COMMAND} -E copy_if_different
+#         ${CMAKE_CURRENT_SOURCE_DIR}/cvutil_core.h ${CMAKE_BINARY_DIR}/library/include/
+#     COMMAND ${CMAKE_COMMAND} -E copy_if_different
+#         ${CMAKE_CURRENT_SOURCE_DIR}/cvutil_matlab_interface.h ${CMAKE_BINARY_DIR}/library/include/
+#     COMMAND ${CMAKE_COMMAND} -E copy_if_different
+#         ${CMAKE_CURRENT_SOURCE_DIR}/cvutil_templates.h ${CMAKE_BINARY_DIR}/library/include/
+#     COMMAND ${CMAKE_COMMAND} -E copy_if_different
+#         ${CMAKE_CURRENT_SOURCE_DIR}/figure.h ${CMAKE_BINARY_DIR}/library/include/
+#     COMMAND ${CMAKE_COMMAND} -E copy_if_different
+#         ${CMAKE_CURRENT_SOURCE_DIR}/profiler.h ${CMAKE_BINARY_DIR}/library/include/
+#     COMMAND ${CMAKE_COMMAND} -E copy_if_different
+#         ${CMAKE_CURRENT_SOURCE_DIR}/stdproto.h ${CMAKE_BINARY_DIR}/library/include/
+#     COMMAND ${CMAKE_COMMAND} -E copy_if_different
+#         ${CMAKE_CURRENT_SOURCE_DIR}/video.h ${CMAKE_BINARY_DIR}/library/include/
+# )


Large blocks of code have been commented out without clear explanation. If this functionality is no longer needed, consider removing it entirely rather than leaving commented code. If it's temporarily disabled for debugging or will be re-enabled later, add a comment explaining why and when it should be restored. This helps maintain code clarity and prevents confusion for future maintainers.

Copilot · 2025-12-02T23:21:29Z

cvutilConfig.cmake.in

+            file(INSTALL \"\${dep}\" DESTINATION \"${target_bin_dir}\")
+            # # Instead of file(INSTALL), use install(FILES ...) to register the file with the manifest.
+            # install(FILES \"\${dep}\" DESTINATION \"${target_bin_dir}\")


The commented-out code suggests using install(FILES ...) instead of file(INSTALL ...) to "register the file with the manifest," but the change reverts back to file(INSTALL ...). The comment indicates that install(FILES ...) is the preferred approach for manifest registration. If file(INSTALL ...) is intentionally being used, consider updating the comment to explain why, or if install(FILES ...) is actually the better choice, the code should use it instead.

Copilot · 2025-12-02T23:21:29Z

CMakeLists.txt

+                  PATHS "${COMPILER_DIR}"
+                        "${COMPILER_DIR}/../../../bin"
+                        "${COMPILER_DIR}/../../bin"
+                        "C:/Program Files/Microsoft Visual Studio/2022/Community/VC/Tools/Llvm/x64/bin"


The hardcoded path "C:/Program Files/Microsoft Visual Studio/2022/Community/VC/Tools/Llvm/x64/bin" may not be portable across different Visual Studio installations. Consider using environment variables like VSINSTALLDIR or make this path configurable through a CMake option to support different Visual Studio editions (Community, Professional, Enterprise) and installation locations.

Copilot · 2025-12-02T23:21:30Z

cvutil/cvutil_bwskel.cpp

+            if (dptr[ploc->getpt() + 1] > dptr[ploc->getpt()] && 
+                iptr[ploc->getpt()] > 0 && ploc->getpt() < (3 * ncols) && 
+                ploc->getimageneighborsum() < 2 && ploc->getimageconnectivity() < 2)
            {
                do
                {
                    ploc->unsetimagepixel();
                    ploc = ploc->right();
-                } while (dptr[ploc->getpt() + 1] > dptr[ploc->getpt()] && iptr[ploc->getpt()] > 0 && ploc->getpt() < (2 * ncols) && ploc->getimageneighborsum() < 2 && ploc->getimageconnectivity() < 2);
+                } while (dptr[ploc->getpt() + 1] > dptr[ploc->getpt()] && 
+                         iptr[ploc->getpt()] > 0 && ploc->getpt() < (3 * ncols) && 
+                         ploc->getimageneighborsum() < 2 && ploc->getimageconnectivity() < 2);


Potential out-of-bounds access: dptr[ploc->getpt() + 1] is accessed without verifying that ploc->getpt() + 1 is within the valid range of the dist matrix. The loop condition checks ploc->getpt() < (3 * ncols), but this doesn't prevent accessing ploc->getpt() + 1 which could exceed bounds when ploc->getpt() equals 3 * ncols - 1. Consider adding a bounds check before accessing dptr[ploc->getpt() + 1].

Fix memory issues and add ASAN to Debug configuration.

b76f8e2

asn5d requested a review from Copilot December 2, 2025 23:15

Copilot started reviewing on behalf of asn5d December 2, 2025 23:15 View session

Copilot finished reviewing on behalf of asn5d December 2, 2025 23:17

Copilot AI reviewed Dec 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix memory issues and add Address Sanitizer to Debug configuration. #4

Fix memory issues and add Address Sanitizer to Debug configuration. #4

Uh oh!

asn5d commented Dec 2, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 2, 2025

Uh oh!

Copilot AI Dec 2, 2025

Uh oh!

Copilot AI Dec 2, 2025

Uh oh!

Copilot AI Dec 2, 2025

Uh oh!

Copilot AI Dec 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Fix memory issues and add Address Sanitizer to Debug configuration. #4

Are you sure you want to change the base?

Fix memory issues and add Address Sanitizer to Debug configuration. #4

Uh oh!

Conversation

asn5d commented Dec 2, 2025

Bug fixes

ASAN Debug configuration

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants