Fixed segmentation fault in Path workbench #9339

xtemp09 · 2023-04-21T09:17:03Z

Closes #6286. This commit fixes segmentation fault in Path workbench.

dubstar-04 introduced the code causing segmentation fault. Later, he removed the debug log and then added try and catch.

evils reported crash in the same issue. The crash occured in the same line of code and everything works if my commit is applied.

I removed try and catch, because I think it is not necessary when comparing two floats.

Standard form

Thank you for creating a pull request to contribute to FreeCAD! Place an "X" in between the brackets below to "check off" to confirm that you have satisfied the requirement, or ask for help in the FreeCAD forum if there is something you don't understand.

Your Pull Request meets the requirements outlined in section 5 of CONTRIBUTING.md for a Valid PR

Please remember to update the Wiki with the features added or changed once this PR is merged.
Note: If you don't have wiki access, then please mention your contribution on the 1.0 Changelog Forum Thread.

xtemp09 · 2023-04-21T09:19:08Z

@evils, ping.

freecadci · 2023-04-21T09:26:26Z

for feature branch PR_9339. Pipeline 844407504 was triggered at b703b01. All CI branches and pipelines.

AjinkyaDahale · 2023-04-21T13:17:11Z

src/Mod/Path/PathSimulator/App/VolSim.cpp

+    if (m_toolShape.empty())
+        return 0.0f;
+
+    toolShapePoint test;
+    test.radiusPos = std::abs(pos) * radius;
+
+    auto it = std::lower_bound(m_toolShape.begin(), m_toolShape.end(), test, toolShapePoint::less_than());
+    return it->heightPos;
 }


The cause of the crash was it being null and still being dereferenced. This is still a possibility if all values in m_toolShape are < test. See https://en.cppreference.com/w/cpp/algorithm/lower_bound. It appears they need m_toolShape to be sorted (or a looser condition defined in the link) before it can be run. If that is already satisfied, the following changes are good to go. If not, there may be a need to do the sorting here (which may be expensive) or elsewhere.

Suggested change

if (m_toolShape.empty())

return 0.0f;

toolShapePoint test;

test.radiusPos = std::abs(pos) * radius;

auto it = std::lower_bound(m_toolShape.begin(), m_toolShape.end(), test, toolShapePoint::less_than());

return it->heightPos;

}

toolShapePoint test;

test.radiusPos = std::abs(pos) * radius;

auto it = std::lower_bound(m_toolShape.begin(), m_toolShape.end(), test, toolShapePoint::less_than());

if (it == m_toolShape.end())

return 0.0;

return it->heightPos;

}

AjinkyaDahale · 2023-04-21T13:19:23Z

src/Mod/Path/PathSimulator/App/VolSim.cpp

@@ -783,14 +783,14 @@ cSimTool::cSimTool(const TopoDS_Shape& toolShape, float res){

 float cSimTool::GetToolProfileAt(float pos)  // pos is -1..1 location along the radius of the tool (0 is center)
 {
-	try{


Removing the try is probably OK, unless the comparison itself can raise exceptions. If it does the catch should also have reported the text of those.

xtemp09 · 2023-04-21T13:49:38Z

Thank you, AjinkyaDahale. Within the try catch there was, simply put, comparison of floats. I found usage of try&catch to compare floats redundant.

AjinkyaDahale · 2023-04-21T13:50:23Z

I haven't tested it yet, but the changes seem fine. @xtemp09 can you confirm the list is sorted?

xtemp09 · 2023-04-21T13:56:09Z

@AjinkyaDahale, the lines 748-775 populate m_toolShape with linearly increased values.

So, yes, it's sorted.

freecadci · 2023-04-21T13:59:18Z

for feature branch PR_9339. Pipeline 844721189 was triggered at 01892d0. All CI branches and pipelines.

AjinkyaDahale · 2023-04-21T14:17:23Z

@AjinkyaDahale, the lines 748-775 populate m_toolShape with linearly increased values.

So, yes, it's sorted.

Could you add a note there (or in the documentation in the header file) mentioning "this must stay sorted for so-and-so reasons" so that if in the future someone creates a non-sorting algorithm, adjustments can be made.

Closes FreeCAD#6286.

xtemp09 · 2023-04-21T15:00:39Z

Done. When running the simulation I found out that the memory allocated for simulation is not deallocated at all.

SUMMARY: LeakSanitizer: 32148253 byte(s) leaked in 6768 allocation(s).

freecadci · 2023-04-21T15:08:34Z

for feature branch PR_9339. Pipeline 844804181 was triggered at 59a208e. All CI branches and pipelines.

sliptonic · 2023-04-25T14:30:21Z

Done. When running the simulation I found out that the memory allocated for simulation is not deallocated at all.
SUMMARY: LeakSanitizer: 32148253 byte(s) leaked in 6768 allocation(s).

Please clarify. Is this another outstanding issue? Are you resolving it?

luzpaz · 2023-04-25T14:34:41Z

@xtemp09 an aside, would you be open to writing a tutorial on how you find leaks so other devs or testers (who don't know) can benefit from utilizing this approach when working on FreeCAD codebase ?

xtemp09 · 2023-04-25T15:31:18Z

sliptonic, luzpaz, I just ran the input files from the issue #6286 and found that the memory allocated to perform simulation is not deallocated at all.

I'm not working on it, because I don't know the mixture of Python and C++ memory model used in FreeCAD. Somebody wiser than me should be dealing with this.

Here is the complete leak-report.txt. Compiled with gcc-12.1 with LSAN, linked against release Python library on Ubuntu 22.04.

The following command was used to compile FreeCAD to find memory leaks

cmake .. -G Ninja \
-D CMAKE_BUILD_TYPE=Debug \
-D CMAKE_INSTALL_PREFIX=/home/xtemp09/mem-test/ \
-D BUILD_ARCH=Off               \
-D BUILD_FEM=Off                \
-D BUILD_OPENSCAD=Off           \
-D BUILD_INSPECTION=Off         \
-D BUILD_PATH=On                \
-D BUILD_REVERSEENGINEERING=Off \
-D BUILD_ROBOT=On               \
-D CMAKE_C_FLAGS_DEBUG="-g -Og -fsanitize=leak -fno-omit-frame-pointer -fuse-ld=mold -Wno-deprecated"   \
-D CMAKE_CXX_FLAGS_DEBUG="-g -Og -fsanitize=leak -fno-omit-frame-pointer -fuse-ld=mold -Wno-deprecated"

If one knows Python, he can add the option

-D PYTHON_LIBRARY_DEBUG=/usr/lib/x86_64-linux-gnu/libpython3.10d.so

When you close a program compiled with leak sanitizer, the leak report goes to the output. I run FreeCAD with the following command:

./FreeCAD > leak-report.txt 2>&1

to output the report into a file.

xtemp09 · 2023-04-25T15:56:09Z

My commentary above provides the command I used to compile FreeCAD to find the memory leaks I mentioned. It works with gcc.

This configuration works with clang

cmake .. -G Ninja \
-D CMAKE_BUILD_TYPE=Debug \
-D CMAKE_INSTALL_PREFIX=/home/xtemp09/path-test/ \
\
-D CMAKE_C_COMPILER=clang -D CMAKE_CXX_COMPILER=clang++                   \
-D CMAKE_C_FLAGS_DEBUG="-glldb -fsanitize=leak -fno-omit-frame-pointer"   \
-D CMAKE_CXX_FLAGS_DEBUG="-glldb -fsanitize=leak -fno-omit-frame-pointer" \
-D LINK_FLAGS_DEBUG="-fsanitize=leak -fuse-ld=mold"                       \
\
-D PYTHON_LIBRARY_DEBUG=/usr/lib/x86_64-linux-gnu/libpython3.10d.so       \
\
-D CMAKE_AR=/usr/lib/llvm-16/bin/llvm-ar               \
-D CMAKE_ADDR2LINE=/usr/lib/llvm-16/bin/llvm-addr2line \
-D CMAKE_MT=/usr/lib/llvm-16/bin/llvm-mt               \
-D CMAKE_NM=/usr/lib/llvm-16/bin/llvm-nm               \
-D CMAKE_OBJCOPY=/usr/lib/llvm-16/bin/llvm-objcopy     \
-D CMAKE_OBJDUMP=/usr/lib/llvm-16/bin/llvm-objdump     \
-D CMAKE_RANLIB=/usr/lib/llvm-16/bin/llvm-ranlib       \
-D CMAKE_READELF=/usr/lib/llvm-16/bin/llvm-readelf     \
-D CMAKE_RC_COMPILER=/usr/lib/llvm-16/bin/llvm-rc      \
-D CMAKE_STRIP=/usr/lib/llvm-16/bin/llvm-strip

sliptonic · 2023-04-26T14:53:54Z

I'm not working on it, because I don't know the mixture of Python and C++ memory model used in FreeCAD. Somebody wiser than me should be dealing with this.

Ok. Thanks. Then I'm inclined to merge this PR. Will please open a new issue to focus on the memory issue so it doesn't get lost?

xtemp09 · 2023-04-26T15:03:31Z

@sliptonic, I've created the forum topic.

sliptonic · 2023-04-26T15:07:47Z

@sliptonic, I've created the forum topic.

You can go straight to a github issue. And mention this PR

sliptonic · 2023-04-26T15:27:45Z

Thanks everyone.

github-actions bot added the WB CAM Related to the CAM/Path Workbench label Apr 21, 2023

AjinkyaDahale reviewed Apr 21, 2023

View reviewed changes

xtemp09 force-pushed the path-fix branch from b703b01 to 01892d0 Compare April 21, 2023 13:45

AjinkyaDahale approved these changes Apr 21, 2023

View reviewed changes

Fixed segmentation fault in Path workbench

59a208e

Closes FreeCAD#6286.

xtemp09 force-pushed the path-fix branch from 01892d0 to 59a208e Compare April 21, 2023 14:58

sliptonic self-assigned this Apr 25, 2023

luzpaz mentioned this pull request Apr 26, 2023

Section on how to find memory leaks in FreeCAD FreeCAD/DevelopersHandbook#43

Open

xtemp09 mentioned this pull request Apr 26, 2023

[Issue] Memory leak in CAM #9403

Open

2 tasks

sliptonic merged commit 9962db4 into FreeCAD:master Apr 26, 2023
7 checks passed

xtemp09 deleted the path-fix branch February 15, 2024 13:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed segmentation fault in Path workbench #9339

Fixed segmentation fault in Path workbench #9339

xtemp09 commented Apr 21, 2023 •

edited

xtemp09 commented Apr 21, 2023

freecadci commented Apr 21, 2023

AjinkyaDahale Apr 21, 2023

AjinkyaDahale Apr 21, 2023

xtemp09 commented Apr 21, 2023

AjinkyaDahale commented Apr 21, 2023

xtemp09 commented Apr 21, 2023 •

edited

freecadci commented Apr 21, 2023

AjinkyaDahale commented Apr 21, 2023

xtemp09 commented Apr 21, 2023 •

edited

freecadci commented Apr 21, 2023

sliptonic commented Apr 25, 2023 •

edited

luzpaz commented Apr 25, 2023

xtemp09 commented Apr 25, 2023

xtemp09 commented Apr 25, 2023 •

edited

sliptonic commented Apr 26, 2023

xtemp09 commented Apr 26, 2023

sliptonic commented Apr 26, 2023

sliptonic commented Apr 26, 2023

Fixed segmentation fault in Path workbench #9339

Fixed segmentation fault in Path workbench #9339

Conversation

xtemp09 commented Apr 21, 2023 • edited

xtemp09 commented Apr 21, 2023

freecadci commented Apr 21, 2023

AjinkyaDahale Apr 21, 2023

Choose a reason for hiding this comment

AjinkyaDahale Apr 21, 2023

Choose a reason for hiding this comment

xtemp09 commented Apr 21, 2023

AjinkyaDahale commented Apr 21, 2023

xtemp09 commented Apr 21, 2023 • edited

freecadci commented Apr 21, 2023

AjinkyaDahale commented Apr 21, 2023

xtemp09 commented Apr 21, 2023 • edited

freecadci commented Apr 21, 2023

sliptonic commented Apr 25, 2023 • edited

luzpaz commented Apr 25, 2023

xtemp09 commented Apr 25, 2023

xtemp09 commented Apr 25, 2023 • edited

sliptonic commented Apr 26, 2023

xtemp09 commented Apr 26, 2023

sliptonic commented Apr 26, 2023

sliptonic commented Apr 26, 2023

xtemp09 commented Apr 21, 2023 •

edited

xtemp09 commented Apr 21, 2023 •

edited

xtemp09 commented Apr 21, 2023 •

edited

sliptonic commented Apr 25, 2023 •

edited

xtemp09 commented Apr 25, 2023 •

edited