Fix writes/reads outside allocated memory in resource constraints shortest paths #93

pmateusz · 2017-07-05T13:11:25Z

Found an example where existing algorithm performs writes/reads outside allocated memory when using the default allocator. Implemented repo as a test included in the pull request. Valgrind reports 38 reads/writes outside allocated memory when running the test against the original version of the algorithm.

I did not get a segmentation fault on my machine, but the algorithm returned a list of edges that do not form a path (example: 1-5 5-2 2-3 4-0).

Investigation
If an existing label is found dominated it becomes immediately deallocated. However, pointers to that label still may exist in other partial paths. Further new allocations cause appearance of invalid paths. The assumption that a pareto optimal path consists only of non-dominated labels and therefore deallocating them is not correct (safe) if the dominance function performs weak inequality checks (as suggested by original author and presented in examples).

Fix

Replaced a raw pointer to r_c_shortest_paths_label with boost::shared_ptr. The algorihtm finds shortest paths based on feasibility/dominance criteria specified by user, so there is a risk that pointers may form a cycle. Taking this into account the algorihtm resets pointers as the final step. I considered using weak pointers in the r_c_shortest_paths_label, but there are examples where such approach do not work. Provided it as a comment inside the algorithm code.
Replaced ks_smart_pointer by boost::shared_ptr. The former class did not offer the smart_pointer functionality and is not necessary.
Removed the b_is_valid checks. It is possible to come up with examples, where this approach will report false negatives (provided an example in comments). It is the same reason why weak_pointers will not work.
Added a test case to prevent regressions.

Valgrind does not detect any leaks or possible leaks when running the test against the fixed algorithm.

jeremy-murphy · 2018-07-25T11:43:17Z

Hi Mateusz. This sounds like a pretty serious bug and an important fix. I'm not an expert on this algorithm, but I've looked pretty closely through the code and it all looks good.
Sorry, what is the "reporo" that you refer to as a test?
Have you done any benchmarks just to check that the change hasn't inadvertently been deleterious to performance? (I personally wouldn't care about minor variation.)
I'm not the maintainer, btw, I just decided to start actively caring about a library that I use. :)

jeremy-murphy · 2018-07-25T11:44:23Z

test/r_c_shortest_paths_test.cpp

+                                  res_cont_2.marked.end() );
+    }
+};
+


I think some brief comments on these new classes would help the reviewers understand what they do.

jeremy-murphy · 2018-07-25T11:45:21Z

include/boost/graph/r_c_shortest_paths.hpp

 // r_c_shortest_paths_label struct
 template<class Graph, class Resource_Container>
-struct r_c_shortest_paths_label
+struct r_c_shortest_paths_label : public boost::enable_shared_from_this<r_c_shortest_paths_label<Graph, Resource_Container> >


I've never used enable_shared_from_this; what does it do?

enable_shared_from_this is a C++11 construct. See the link, it is better explained than I could.

pmateusz · 2018-07-25T16:06:09Z

Hi @jeremy-murphy, I answered to your questions below.

By reporo I mean repro case. Sorry for my typo. I fixed the comment. I was referring to the new test case I added.
Good questions about performance tests. I didn't do them. Changes proposed are to replace the proprietary smart pointer with the boost one. Moreover, I am not proposing an improvement, rather a fix for critical issue, so possible performance impact should be justifiable.

jeremy-murphy · 2018-07-27T23:59:54Z

You're right, and I didn't mean to cast any doubt about performance, it's just a standard question. I assume you would have noticed if it was 10x slower, which is the kind of accidental performance bug I mean.
Well, I hope a maintainer has time to review it soon, as I think it really deserves to be merged asap.
@Belcourt are you currently the right person to gently remind about these PRs?

pmateusz · 2018-07-29T16:58:03Z

@jeremy-murphy, it seems there is a ticket https://svn.boost.org/trac10/ticket/11723 with a similar issue. I asked the owner for a suggestion about next steps.

anadon · 2018-08-31T17:52:57Z

I'm helping out with the PR backlog. Looks like you have a bugfix, test, and no new examples will be needed. This will be in the first batch of merged PR's. I will need to take time to understand this issue and fix more. This is to let you know and help me prioritize PR's.

Belcourt · 2018-08-31T17:57:16Z

I'm having trouble building graph / develop following all these PR's. Do you, by chance, test each PR by building and running graph, graph parallel, and MPI tests before pushing it? It might be wise to slow down so we don't irreversibly break something.

anadon · 2018-08-31T18:53:20Z

Right now I'm letting people know that something is happening, adding some documentation, and prioritizing PR's to mess with. Some of these old PR's need to be rebased so I'm stirring things up a little bit to get the submitters to check in and to know things are happening. I'll be reading, testing, merging, and testing again PR's I say will be in the first batch of merges

pdimov · 2018-09-29T23:00:33Z

include/boost/graph/r_c_shortest_paths.hpp

  ( const unsigned long n, 
    const Resource_Container& rc = Resource_Container(), 
-    const r_c_shortest_paths_label* const pl = 0, 
+    const boost::shared_ptr<r_c_shortest_paths_label<Graph, Resource_Container> > pl = nullptr, 


nullptr is C++11, the C++03 tests will fail.

This was fixed in #114.

It's better to fix it here, if possible. It's easier to review and merge PRs one by one (and it would be easier to revert them later if some prove problematic.)

You can. I'm new and don't have write access so having one place to work on keeps things a tad more sane. It fixing commit was 6808062 .

No, I can't fix people's PRs, they have to do that themselves. :-)

I'm trying to save you trouble in the long run. Batching up unrelated changes in one PR is not good workflow. It's better to either request the needed PR changes from the original submitters, who are usually happy to help, or, if you want to fix their PRs yourself, you could create one new PR per each original PR, so that the changes can be reviewed and applied in isolation.

Thanks, @pdimov, I reapplied @anadon commit that removes usage of nullptr and removes spaces from end of lines.

pdimov · 2018-09-29T23:02:53Z

test/r_c_shortest_paths_test.cpp

+        if( this == &other )
+            return *this;
+        this->~spp_spptw_marked_res_cont();
+        new( this ) spp_spptw_marked_res_cont( other );


Invoking the destructor and then the constructor is not a good way to write an assignment operator. Perhaps it works here, I haven't checked, but it's not a good practice and it would be better to rewrite it in a more conventional way.

@pdimov, I agree. I implemented proper assignment operators in tests whenever possible. Only one has left in https://github.com/boostorg/graph/blob/develop/include/boost/graph/r_c_shortest_paths.hpp#L47. See this is part of released Boost. It was done by the original developer of the feature to allow reassignment of fields marked as const in the struct definition.

I was not changing it deliberately, because it has been already released and it is not causing any bugs as far as I am concerned. It is quite unusual construct to use without a reason.

anadon · 2018-10-07T06:43:40Z

Try rebasing on the current develop branch. I think some of your changes were already added so it won't be trivial to merge.

…rind confirms reads/writes outside allocated memory.

…- explain a scenario where they do not work correctly.

…t shared pointer constructor

pmateusz · 2018-10-07T22:31:55Z

Recent TravisCI failure in a single build is a timeout in the mas_test.cpp test, which is unrelated to changes in this thread.

pdimov · 2018-10-07T23:20:41Z

I restarted the timed-out job and it passed, so it's all clear now.

jzmaddock · 2018-10-12T17:38:34Z

Are these changes in #114? If so it can be closed now.

anadon · 2018-10-12T18:14:13Z

These changes were included in #114 and so this should be closed.

jeremy-murphy reviewed Jul 25, 2018

View reviewed changes

anadon mentioned this pull request Sep 1, 2018

First batch of merges from the backlog. #114

Merged

pdimov reviewed Sep 29, 2018

View reviewed changes

Mateusz Polnik and others added 6 commits October 7, 2018 12:54

Reproduce memory issue with resource constrained shortest paths. Valg…

d8ccbc1

…rind confirms reads/writes outside allocated memory.

Fix read/writes outside allocated memory. Remove is_valid assertions …

87d1312

…- explain a scenario where they do not work correctly.

Remove whitespaces at the end of lines and repace nullptr by a defaul…

b171f86

…t shared pointer constructor

Simplify if statements

87fcb8b

Implement proper assingment operator

56a4a8d

Remove typename used outside template context

992e688

jzmaddock closed this Oct 12, 2018

Fix writes/reads outside allocated memory in resource constraints shortest paths #93

Fix writes/reads outside allocated memory in resource constraints shortest paths #93

Uh oh!

Conversation

pmateusz commented Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeremy-murphy commented Jul 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmateusz commented Jul 25, 2018

Uh oh!

jeremy-murphy commented Jul 27, 2018

Uh oh!

pmateusz commented Jul 29, 2018

Uh oh!

anadon commented Aug 31, 2018

Uh oh!

Belcourt commented Aug 31, 2018

Uh oh!

anadon commented Aug 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anadon commented Oct 7, 2018

Uh oh!

pmateusz commented Oct 7, 2018

Uh oh!

pdimov commented Oct 7, 2018

Uh oh!

jzmaddock commented Oct 12, 2018

Uh oh!

anadon commented Oct 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pmateusz commented Jul 5, 2017 •

edited

Loading