search

spencer-project / spencer_people_tracking

Multi-modal ROS-based people detection and tracking framework for mobile robots developed within the context of the EU FP7 project SPENCER.
http://www.spencer.eu/
660 stars 327 forks source link

Fixed - Upper_body_detection segfault, memory corruption #12

Closed rentt closed 8 years ago

rentt commented 8 years ago

This is a patch about issue#10. Most segfault are caused by double free. This patch fixed some programming level bugs without compromising function.

NOTE: The app still crashed time and again, the "respawn='true'" still need in the launch file. I guess the bugs locate in the function EvaluateTemplate() in file detector.cpp. Will continue struggle with it...

rentt commented 8 years ago

Finally, fixed a bug lead to crashed in the function EvaluateTemplate(). Maybe there is still bug exist. But the program running stable for 4 hours in my testing.

lucasb-eyer commented 8 years ago

Thank you for contributing back. Few comments:

  1. You're mixing tabs and spaces, which makes this especially hard to read.
  2. Why does changing the Matrix/Vector return values to parameters fix anything? I don't think it does. More importantly, some of these changes are actually wrong, for example that to conv1D.
  3. I had the same idea as you, that the KLinkedList destructor is probably wrong. I looked into it for a long time, and turns out it's actually correct. It's one of the most confusing/convoluted/bad pieces of code ever, but I think it's correct.
  4. The extra-checks in some if-statements make sense.
  5. The off-by-one fix in EvaluateTemplate may or may not be correct, I really don't know and I doubt anyone knows :-/
rentt commented 8 years ago

1) fixed, but only fix the statement which I have modified. 2) If we take a local variate as a return value, the memory will be free twice, as known "double free". I feel strange when I saw these codes in the spencer project. Then I wrote a little testing app which used Matrix/Vector like in the upper_body codes. And I got crash with "double free".
3,4) I didn't get crashed in these statements. The codes I added just for safety. 5) When I run the app in the debug model with GDB, and got segment fault with assert statement in Matrix::operator() function, which was called in function EvaluateTemplate(line 297:if(depth_map(ii_depth,jj_depth) <= min_distance_threshold || depth_map(ii_depth,jj_depth) >= max_distance_threshold)) The depth_map is (640, 480) while cropped is (641, 481). Then the assert be triggered.

rentt commented 8 years ago

Thanks for the comments. Fixed this bug. I removed this function in my testing. This function not used in project, instead the project use another conv1D with an addition parameter "dirFlag".

lucasb-eyer commented 8 years ago

Even then, Vector has correct copy behaviour (it's only attribute is a std::vector), so it is valid to use it as a return value.

rentt commented 8 years ago

Yes, I agree. I think it should be better using a reference instead return value. Especially used on big data(class) for efficiency(memory copy). Currently, I only make the upper_body not crash. But the codes need to be optimized like using the 3rd math library and etc. I have made a new PR#14 based on the latest codes of spencer_people_tracking:master. It has been well tested on a robot in the office.

lucasb-eyer commented 8 years ago

No, what I'm saying is that the return value is correct. It is not a reason of the crash.

I do agree with you that much of the code is terrible for many reasons, but changing to a reference will also not speed it up. Modern compilers do NRVO here and so there is essentially no copy in most, if not all of these cases. So the original implementor got lucky that compilers are so smart ;-)