Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix a stack-use-after-scope in Arr_polycurve_basic_traits_2 #4012

Closed
wants to merge 1 commit into from
Closed

Fix a stack-use-after-scope in Arr_polycurve_basic_traits_2 #4012

wants to merge 1 commit into from

Conversation

lrineau
Copy link
Member

@lrineau lrineau commented Jun 18, 2019

Summary of Changes

Before this patch, Arr_polycurve_basic_traits_2 was holding a const
reference to a temporary object.

Here was the result of ctest, with the address sanitizer:

$ make && ASAN_OPTIONS=detect_leaks=0 ctest -L Arr -j6
[...]
The following tests FAILED:
        144 - execution___of__generic_curve_data (Failed)
        170 - execution___of__polycurve_circular_arc (Failed)
        172 - execution___of__polycurve_conic (Failed)
        176 - execution___of__polycurves_basic (Failed)
        178 - execution___of__polylines (Failed)
        2166 - test_traits_test_polylines__data_polylines_compare_y_at_x__polyline_traits (Failed)
        2168 - test_traits_test_polylines__data_polylines_intersect__polyline_traits (Failed)
        2169 - test_traits_test_polylines__data_polylines_split__polyline_traits (Failed)
        2171 - test_traits_test_polylines__data_polylines_assertions__polyline_traits (Failed)
        2175 - test_traits_conic_polycurve__data_polycurves_conics_compare_y_at_x__polycurve_conic_traits (Failed)
        2176 - test_traits_conic_polycurve__data_polycurves_conics_compare_y_at_x_left__polycurve_conic_traits (Failed)
        2177 - test_traits_conic_polycurve__data_polycurves_conics_compare_y_at_x_right__polycurve_conic_traits (Failed)
        2179 - test_traits_conic_polycurve__data_polycurves_conics_intersect__polycurve_conic_traits (Failed)
        2180 - test_traits_conic_polycurve__data_polycurves_conics_split__polycurve_conic_traits (Failed)
        2193 - test_traits_circular_arc_polycurve__data_Polycurves_circular_arcs_compare_y_at_x__polycurve_circular_arc_traits (Failed)
        2198 - test_traits_circular_arc_polycurve__data_Polycurves_circular_arcs_split__polycurve_circular_arc_traits (Failed)
        2221 - test_traits_non_caching_polylines__data_polylines_compare_y_at_x__non_caching_polyline_traits (Failed)
        2223 - test_traits_non_caching_polylines__data_polylines_intersect__non_caching_polyline_traits (Failed)
        2224 - test_traits_non_caching_polylines__data_polylines_split__non_caching_polyline_traits (Failed)
        2226 - test_traits_non_caching_polylines__data_polylines_assertions__non_caching_polyline_traits (Failed)
        2400 - execution___of__test_construction_polylines (Failed)
        2477 - execution___of__test_io (Failed)

This is fixed rather easily, by copying the traits class, instead of
holding a reference to it.

Release Management

  • Affected package(s): Arr_2
  • License and copyright ownership: maintenance by GeometryFactory

@lrineau
Fix a stack-use-after-scope in Arr_polycurve_basic_traits_2
789cf86 
Before this patch, `Arr_polycurve_basic_traits_2` was holding a const
reference to a temporary object.

Here was the result of ctest, with the address sanitizer:
```shellsession
$ make && ASAN_OPTIONS=detect_leaks=0 ctest -L Arr -j6
[...]
The following tests FAILED:
        144 - execution___of__generic_curve_data (Failed)
        170 - execution___of__polycurve_circular_arc (Failed)
        172 - execution___of__polycurve_conic (Failed)
        176 - execution___of__polycurves_basic (Failed)
        178 - execution___of__polylines (Failed)
        2166 - test_traits_test_polylines__data_polylines_compare_y_at_x__polyline_traits (Failed)
        2168 - test_traits_test_polylines__data_polylines_intersect__polyline_traits (Failed)
        2169 - test_traits_test_polylines__data_polylines_split__polyline_traits (Failed)
        2171 - test_traits_test_polylines__data_polylines_assertions__polyline_traits (Failed)
        2175 - test_traits_conic_polycurve__data_polycurves_conics_compare_y_at_x__polycurve_conic_traits (Failed)
        2176 - test_traits_conic_polycurve__data_polycurves_conics_compare_y_at_x_left__polycurve_conic_traits (Failed)
        2177 - test_traits_conic_polycurve__data_polycurves_conics_compare_y_at_x_right__polycurve_conic_traits (Failed)
        2179 - test_traits_conic_polycurve__data_polycurves_conics_intersect__polycurve_conic_traits (Failed)
        2180 - test_traits_conic_polycurve__data_polycurves_conics_split__polycurve_conic_traits (Failed)
        2193 - test_traits_circular_arc_polycurve__data_Polycurves_circular_arcs_compare_y_at_x__polycurve_circular_arc_traits (Failed)
        2198 - test_traits_circular_arc_polycurve__data_Polycurves_circular_arcs_split__polycurve_circular_arc_traits (Failed)
        2221 - test_traits_non_caching_polylines__data_polylines_compare_y_at_x__non_caching_polyline_traits (Failed)
        2223 - test_traits_non_caching_polylines__data_polylines_intersect__non_caching_polyline_traits (Failed)
        2224 - test_traits_non_caching_polylines__data_polylines_split__non_caching_polyline_traits (Failed)
        2226 - test_traits_non_caching_polylines__data_polylines_assertions__non_caching_polyline_traits (Failed)
        2400 - execution___of__test_construction_polylines (Failed)
        2477 - execution___of__test_io (Failed)
```

This is fixed rather easily, by copying the traits class, instead of
holding a reference to it.
@lrineau lrineau added this to the 4.14.1 milestone Jun 18, 2019
@lrineau lrineau requested a review from efifogel June 18, 2019 09:34
@lrineau lrineau self-assigned this Jun 18, 2019
@lrineau
Copy link
Member Author

lrineau commented Jun 18, 2019

Note that I have used AddressSanitizer with the option detect_leaks=0, because the testsuite of Arr_2 also have a lot of leaks (according to AddressSanitizer), but that is another matter.

@lrineau lrineau mentioned this pull request Jun 18, 2019
Merged
@maxGimeno
Copy link
Contributor

errors

@lrineau
Copy link
Member Author

lrineau commented Jul 1, 2019

Those errors are because of my other PR #4013. But let's block both PRs, until I can find a solution.

@efifogel
Copy link
Member

efifogel commented Jul 1, 2019 via email

@lrineau
Copy link
Member Author

lrineau commented Jul 1, 2019

@efifogel commented on Jul 1, 2019, 2:04 PM GMT+2:

Hi Laurent,

I think that I've managed to skip your request from 2 weeks ago, but at
least I see it now.

The fix is somehow incorrect.
Some of our traits have state data; thus, we do not copy traits (only pass
references).

I understand that traits can have state data, but I thought they are probably constant state data, that can be copied.

Frankly, I don't understand why does the tool you used reports an error,
but perhaps you can try skipping the definition and usage of the local
variable 'geom_traits'.
In particular:

Instead of:
const Subcurve_traits_2* geom_traits = subcurve_traits_2();
Compare_points<Compare_x_2> compare(geom_traits, compare_x_2_object(), q);

Do:
Compare_points<Compare_x_2> compare(subcurve_traits_2(),
compare_x_2_object(), q);

I do not understand the suggestion. The reason AddressSanitizer reported an error is that the compare object was holding a reference to a temporary object already destroyed.

I was trying your command:

make && ASAN_OPTIONS=detect_leaks=0 ctest -L Arr -j6

but did not get errors. How do I reproduce?

You have to enable first the AddressSanitizer. It can be used with any non-ancient versions of gcc and clang, by adding -fsanitize=adress to the compilation flags.

@lrineau lrineau added the Not yet approved The feature or pull-request has not yet been approved. label Jul 1, 2019
@efifogel
Copy link
Member

efifogel commented Jul 1, 2019 via email

@lrineau
Copy link
Member Author

lrineau commented Jul 1, 2019

-fsanitize=address, with 2 d at address. With only one, the compiler probably warns that the option does not exist.

@lrineau
Copy link
Member Author

lrineau commented Jul 2, 2019

On a run of:

test_traits_test_polylines "data/polylines/compare_y_at_x.pt" "data/polylines/compare_y_at_x.xcv" "data/empty.zero" "data/polylines/compare_y_at_x" "polyline_traits"

then the full error from AddressSanitizer is:

Performing test: traits type is polyline_traits, input files are data/polylines/compare_y_at_x.pt data/polylines/compare_y_at_x.xcv data/empty.zero data/polylines/compare_y_at_x

case number : 1
Test: compare_y_at_x( 0/1 0/1,3 0/1 0/1 0/1 2/1 0/1 4/1 0/1 6/1 ) ? 0 Passed
case number : 2
Test: compare_y_at_x( 0/1 1/1,3 0/1 0/1 0/1 2/1 0/1 4/1 0/1 6/1 ) ? 0 Passed
case number : 3
Test: compare_y_at_x( 0/1 2/1,3 0/1 0/1 0/1 2/1 0/1 4/1 0/1 6/1 ) ? 0 Passed
case number : 4
Test: compare_y_at_x( 0/1 3/1,3 0/1 0/1 0/1 2/1 0/1 4/1 0/1 6/1 ) ? 0 Passed
case number : 5
Test: compare_y_at_x( 0/1 4/1,3 0/1 0/1 0/1 2/1 0/1 4/1 0/1 6/1 ) ? 0 Passed
case number : 6
Test: compare_y_at_x( 0/1 5/1,3 0/1 0/1 0/1 2/1 0/1 4/1 0/1 6/1 ) ? 0 Passed
case number : 7
Test: compare_y_at_x( 0/1 6/1,3 0/1 0/1 0/1 2/1 0/1 4/1 0/1 6/1 ) ? 0 Passed
case number : 8
Test: compare_y_at_x( 0/1 -1/1,3 0/1 0/1 0/1 2/1 0/1 4/1 0/1 6/1 ) ? 4294967295 Passed
case number : 9
Test: compare_y_at_x( 0/1 7/1,3 0/1 0/1 0/1 2/1 0/1 4/1 0/1 6/1 ) ? 1 Passed
case number : 10
Test: compare_y_at_x( 0/1 -1/1,2 0/1 0/1 2/1 2/1 4/1 4/1 ) ? 4294967295 
=================================================================
==20046==ERROR: AddressSanitizer: stack-use-after-scope on address 0x7fffffffbcc0 at pc 0x00000055ee41 bp 0x7fffffffb7f0 sp 0x7fffffffb7e8
READ of size 8 at 0x7fffffffbcc0 thread T0
    #0 0x55ee40 in CGAL::Arr_polycurve_basic_traits_2<CGAL::Arr_segment_traits_2<CGAL::Cartesian<CGAL::Gmpq> > >::subcurve_traits_2() const /home/lrineau/Git/other-cgal2/Arrangement_on_surface_2/include/CGAL/Arr_polycurve_basic_traits_2.h:128:12
    #1 0x5721eb in CGAL::Arr_polycurve_basic_traits_2<CGAL::Arr_segment_traits_2<CGAL::Cartesian<CGAL::Gmpq> > >::Compare_points<CGAL::Arr_polycurve_basic_traits_2<CGAL::Arr_segment_traits_2<CGAL::Cartesian<CGAL::Gmpq> > >::Compare_x_2>::operator()(CGAL::Arr_segment_2<CGAL::Cartesian<CGAL::Gmpq> > const&, CGAL::Arr_curve_end) /home/lrineau/Git/other-cgal2/Arrangement_on_surface_2/include/CGAL/Arr_polycurve_basic_traits_2.h:2559:60
    #2 0x571892 in unsigned long CGAL::Arr_polycurve_basic_traits_2<CGAL::Arr_segment_traits_2<CGAL::Cartesian<CGAL::Gmpq> > >::locate_gen<CGAL::Arr_polycurve_basic_traits_2<CGAL::Arr_segment_traits_2<CGAL::Cartesian<CGAL::Gmpq> > >::Compare_points<CGAL::Arr_polycurve_basic_traits_2<CGAL::Arr_segment_traits_2<CGAL::Cartesian<CGAL::Gmpq> > >::Compare_x_2> >(CGAL::internal::X_monotone_polycurve_2<CGAL::Arr_segment_2<CGAL::Cartesian<CGAL::Gmpq> >, CGAL::Point_2<CGAL::Cartesian<CGAL::Gmpq> > > const&, CGAL::Arr_polycurve_basic_traits_2<CGAL::Arr_segment_traits_2<CGAL::Cartesian<CGAL::Gmpq> > >::Compare_points<CGAL::Arr_polycurve_basic_traits_2<CGAL::Arr_segment_traits_2<CGAL::Cartesian<CGAL::Gmpq> > >::Compare_x_2>) const /home/lrineau/Git/other-cgal2/Arrangement_on_surface_2/include/CGAL/Arr_polycurve_basic_traits_2.h:2485:34
    #3 0x5705a3 in CGAL::Arr_polycurve_basic_traits_2<CGAL::Arr_segment_traits_2<CGAL::Cartesian<CGAL::Gmpq> > >::locate(CGAL::internal::X_monotone_polycurve_2<CGAL::Arr_segment_2<CGAL::Cartesian<CGAL::Gmpq> >, CGAL::Point_2<CGAL::Cartesian<CGAL::Gmpq> > > const&, CGAL::Point_2<CGAL::Cartesian<CGAL::Gmpq> > const&) const /home/lrineau/Git/other-cgal2/Arrangement_on_surface_2/include/CGAL/Arr_polycurve_basic_traits_2.h:2723:12
    #4 0x56f878 in CGAL::Arr_polycurve_basic_traits_2<CGAL::Arr_segment_traits_2<CGAL::Cartesian<CGAL::Gmpq> > >::locate_impl(CGAL::internal::X_monotone_polycurve_2<CGAL::Arr_segment_2<CGAL::Cartesian<CGAL::Gmpq> >, CGAL::Point_2<CGAL::Cartesian<CGAL::Gmpq> > > const&, CGAL::Point_2<CGAL::Cartesian<CGAL::Gmpq> > const&, CGAL::Arr_all_sides_oblivious_tag) const /home/lrineau/Git/other-cgal2/Arrangement_on_surface_2/include/CGAL/Arr_polycurve_basic_traits_2.h:2696:12
    #5 0x56f3f0 in CGAL::Arr_polycurve_basic_traits_2<CGAL::Arr_segment_traits_2<CGAL::Cartesian<CGAL::Gmpq> > >::Compare_y_at_x_2::operator()(CGAL::Point_2<CGAL::Cartesian<CGAL::Gmpq> > const&, CGAL::internal::X_monotone_polycurve_2<CGAL::Arr_segment_2<CGAL::Cartesian<CGAL::Gmpq> >, CGAL::Point_2<CGAL::Cartesian<CGAL::Gmpq> > > const&) const /home/lrineau/Git/other-cgal2/Arrangement_on_surface_2/include/CGAL/Arr_polycurve_basic_traits_2.h:749:25
    #6 0x54c18d in Traits_test<CGAL::Arr_polyline_traits_2<CGAL::Arr_segment_traits_2<CGAL::Cartesian<CGAL::Gmpq> > > >::compare_y_at_x_wrapper(std::__cxx11::basic_istringstream<char, std::char_traits<char>, std::allocator<char> >&) /home/lrineau/Git/other-cgal2/Arrangement_on_surface_2/test/Arrangement_on_surface_2/Traits_test.h:815:5
    #7 0x552e6e in Traits_test<CGAL::Arr_polyline_traits_2<CGAL::Arr_segment_traits_2<CGAL::Cartesian<CGAL::Gmpq> > > >::exec(std::__cxx11::basic_istringstream<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool&) /home/lrineau/Git/other-cgal2/Arrangement_on_surface_2/test/Arrangement_on_surface_2/Traits_test.h:63:14
    #8 0x544290 in Traits_base_test<CGAL::Arr_polyline_traits_2<CGAL::Arr_segment_traits_2<CGAL::Cartesian<CGAL::Gmpq> > > >::perform() /home/lrineau/Git/other-cgal2/Arrangement_on_surface_2/test/Arrangement_on_surface_2/Traits_base_test.h:306:21
    #9 0x53ffd2 in main /home/lrineau/Git/other-cgal2/Arrangement_on_surface_2/test/Arrangement_on_surface_2/test_traits.cpp:116:13
    #10 0x7ffff77a9f32 in __libc_start_main /usr/src/debug/glibc-2.29-24-g2ec0b166bf/csu/../csu/libc-start.c:308:16
    #11 0x42315d in _start (/home/lrineau/Git/other-cgal2/build-address-sanitizer/test/Arrangement_on_surface_2/test_traits_test_polylines+0x42315d)

Address 0x7fffffffbcc0 is located in stack of thread T0 at offset 384 in frame
    #0 0x56fd2f in CGAL::Arr_polycurve_basic_traits_2<CGAL::Arr_segment_traits_2<CGAL::Cartesian<CGAL::Gmpq> > >::locate(CGAL::internal::X_monotone_polycurve_2<CGAL::Arr_segment_2<CGAL::Cartesian<CGAL::Gmpq> >, CGAL::Point_2<CGAL::Cartesian<CGAL::Gmpq> > > const&, CGAL::Point_2<CGAL::Cartesian<CGAL::Gmpq> > const&) const /home/lrineau/Git/other-cgal2/Arrangement_on_surface_2/include/CGAL/Arr_polycurve_basic_traits_2.h:2700

  This frame has 12 object(s):
    [32, 40) 'geom_traits' (line 2701)
    [64, 65) 'ref.tmp' (line 2702)
    [80, 81) 'min_vertex' (line 2704)
    [96, 104) 'compare_x' (line 2706)
    [128, 152) 'compare' (line 2714)
    [192, 208) 'ref.tmp17' (line 2714)
    [224, 232) 'agg.tmp'
    [256, 280) 'agg.tmp22'
    [320, 344) 'compare34' (line 2722)
    [384, 400) 'ref.tmp35' (line 2722) <== Memory access at offset 384 is inside this variable
    [416, 424) 'agg.tmp36'
    [448, 472) 'agg.tmp43'

As the report says, the temporary object on the stack is created here in CGAL::Arr_polycurve_basic_traits_2<CGAL::Arr_segment_traits_2<K>>::locate:

cgal/Arrangement_on_surface_2/include/CGAL/Arr_polycurve_basic_traits_2.h

Lines 2696 to 2715 in 12da8d1

std::size_t locate(const X_monotone_curve_2& xcv, const Point_2& q) const
{
const Subcurve_traits_2* geom_traits = subcurve_traits_2();
if (geom_traits->is_vertical_2_object()(xcv[0])) {
// Verify that q has the same x-coord as cv (which is vertical)
typename Subcurve_traits_2::Construct_min_vertex_2 min_vertex =
geom_traits->construct_min_vertex_2_object();
typename Subcurve_traits_2::Compare_x_2 compare_x =
geom_traits->compare_x_2_object();
Comparison_result res = compare_x(min_vertex(xcv[0]), q);
if (res != EQUAL) return INVALID_INDEX;
Compare_points<Compare_xy_2> compare(geom_traits,
compare_xy_2_object(), q);
return locate_gen(xcv, compare);
}
Compare_points<Compare_x_2> compare(geom_traits, compare_x_2_object(), q);
return locate_gen(xcv, compare);
}

The problem is that the type of the pointer geom_traits is:

CGAL::Arr_segment_traits_2<CGAL::Cartesian<CGAL::Gmpq> > const*

and it is passed as first argument to the constructor of compare,

cgal/Arrangement_on_surface_2/include/CGAL/Arr_polycurve_basic_traits_2.h

Lines 2545 to 2550 in 12da8d1

Compare_points(const Polycurve_basic_traits_2& traits, Comparer compare,
const Point_2& p) :
m_poly_traits(traits),
m_point(p),
m_compare(compare)
{}

and the first parameter is a const& to a type:

CGAL::Arr_polycurve_basic_traits_2<CGAL::Arr_segment_traits_2<CGAL::Cartesian<CGAL::Gmpq> > >

So, how does that work?! The argument is a pointer, and the parameter is an const& to an object! The reason is that CGAL::Arr_polycurve_basic_traits_2 has an implicit constructor with one argument:

cgal/Arrangement_on_surface_2/include/CGAL/Arr_polycurve_basic_traits_2.h

Lines 102 to 103 in 12da8d1

Arr_polycurve_basic_traits_2(const Subcurve_traits_2* geom_traits) :
m_subcurve_traits(geom_traits), m_own_traits(false) {}

So, on the fly, a temporary CGAL::Arr_polycurve_basic_traits_2 is created, bound to a const reference, and then automatically destroyed just after that! Once we unfold all that, I am surprise that the compiler does not warn about that issue.

My patch makes the CGAL::Arr_polycurve_basic_traits_2 be copied instead of hold as a const reference. And I think that is fine, because that class CGAL::Arr_polycurve_basic_traits_2 only contains a pointer and a bool:

cgal/Arrangement_on_surface_2/include/CGAL/Arr_polycurve_basic_traits_2.h

Lines 84 to 86 in 12da8d1

// Data members:
const Subcurve_traits_2* m_subcurve_traits; // The base segment-traits class.
bool m_own_traits;

so at most that is two words to copy (16 octets in case of a 64 bits architecture).

@efifogel
Copy link
Member

efifogel commented Jul 2, 2019 via email

@efifogel
Copy link
Member

efifogel commented Jul 2, 2019 via email

@lrineau
Copy link
Member Author

lrineau commented Jul 3, 2019

@efifogel commented on Jul 2, 2019, 9:16 PM GMT+2:

I've tried
cmake -DCMAKE_CXX_FLAGS="-frounding-math -Wall -g -fsanitize=address
-fno-omit-frame-pointer"
and
cmake -DCMAKE_BUILD_TYPE=ASAN
but, nothing
I'm using g++ 5.4.0.
Do I have to use clang?

AddressSanitizer is part of gcc since version 4.8 and part of LLVM/clang since version 3.1. Your version 5.4 of gcc should be sufficient. But maybe its copy of AddressSanitizer is too old to detect the issue. If you have a more recent version of gcc, or clang, try it.

To enable it, you just have to add -fsanitize=address to CMAKE_CXX_FLAGS, as you did:

cmake -DCMAKE_CXX_FLAGS="-frounding-math -Wall -g -fsanitize=address -fno-omit-frame-pointer"

However ASAN is not a valid value for CMAKE_BUILD_TYPE. You can leave it empty, or set it to Release or Debug.

@efifogel
Copy link
Member

efifogel commented Jul 7, 2019 via email

@lrineau
Copy link
Member Author

lrineau commented Jul 9, 2019

I have verified that your patch does not fix the issue. The temporary object of type Arr_polycurve_basic_traits_2 is still created, and the reference bound to it.

@efifogel
Copy link
Member

efifogel commented Jul 9, 2019 via email

@efifogel
Copy link
Member

efifogel commented Jul 9, 2019 via email

@maxGimeno
Copy link
Contributor

Successfully tested in https://cgal.geometryfactory.com/CGAL/testsuite/results-5.0-Ic-95.shtml

@maxGimeno
Copy link
Contributor

Successfully tested in https://cgal.geometryfactory.com/CGAL/testsuite/results-4.14.1-Ic-98.shtml

@lrineau
Copy link
Member Author

lrineau commented Jul 31, 2019

This PR is still not yet approved by Efi. Let's stop testing it for the moment.

@lrineau lrineau removed this from the 4.14.1 milestone Sep 18, 2019
@lrineau
Copy link
Member Author

lrineau commented Sep 18, 2019

Replaced by #4230.

@lrineau lrineau closed this Sep 18, 2019
@lrineau lrineau deleted the Arr_2-fix_polycurve_traits-GF branch September 18, 2019 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@efifogel efifogel Awaiting requested review from efifogel

Assignees

@lrineau lrineau

@maxGimeno maxGimeno

Labels
Bug Not yet approved The feature or pull-request has not yet been approved. Pkg::Arrangement_on_surface_2 Replaced tested manually on Linux
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@lrineau @maxGimeno @efifogel