Geometry Boost List: Re: [geometry] access violation in update_discarded(...)

Subject: Re: [geometry] access violation in update_discarded(...)
From: Barend Gehrels (barend_at_[hidden])
Date: 2014-11-15 06:26:14

Hi Volker,

Thanks for your clear message ;-)

Volker Schöch wrote On 14-11-2014 15:28:
>
> Hi Barend,
>
> Thank you for your recent reply. I meant to answer all your questions 
> in one mail, which took a while, so please bear with me while I try to 
> sort it all out.
>
> 1) I think I have to write: it is volunteer work - the ticket is 
> opened one week ago. We appreciate your usage of Boost.Geometry and 
> the feedback too, but take care not to transfer the pressure you feel 
> to us volunteers... At least not too soon.
>
> Sure, absolutely. I can't tell you how much I appreciate your work and 
> your availability! Honestly. By "we" I was referring to think-cell: We 
> have to deal with these problems one way or another. Of course, before 
> spending time on working around problems, it's tremendously helpful to 
> understand if and when there might be a real fix available. I 
> apologize if my way of putting this into words was ambiguous. No harm 
> meant!
>

Thanks - we appreciate all your messages and researches. There is no 
harm of course, I just wanted to subtle indicate something about 
pressure but maybe it was not subtle enough... Anyway, no problem at all.

>
> 2) Menelaos senta sample program which works for him, and works for 
> me. Did you try that program? He also asked
> Do you think you can reproduce the problem in a standalone program?
>
> but you did not answer that question. Actually the whole mail was 
> unanswered.
>
> The fact that the algorithm accesses uninitialized memory means two 
> things:
>
> -The problem may be hard to reproduce.
>
> -Just because it happens to work under certain circumstances, doesn't 
> mean it's correct.
>
> For this reason, instead of insisting on the fact that it keeps 
> crashing on my machine when compiled with my compiler, my colleague 
> Valentin made an effort and stepped into the algorithm and found some 
> code where you were (in 1.56.0) accessing uninitialized memory. 
> Whether or not that produces a crash later, depends on the actual 
> values found in that uninitialized memory. For example, most compilers 
> in fact do initialize "uninitialized" memory with a certain value 
> pattern, when compiling with debug build settings, and so when testing 
> you may actually reliably read the same value that doesn't lead to a 
> crash further down. But that doesn't mean it's correct, and it may 
> still crash, e.g., with my release build compiler settings.
>
> For all these reasons, I deemed Valentins email an appropriate answer 
> to Menelaos' question.
>

The answer was helpful indeed.

>
> 3) you already mention that your input is invalid - so you should be 
> able to workaround the problem probably easily.
>
> Yes, you're right. Running the new is_valid(...) predicate before 
> invoking the actual algorithm helps to suppress the problem in 1.56.0. 
> I'm still struggling with your ideas of valid/invalid, pre conditions 
> and post conditions. Let's postpone that topic to a separate discussion.
>
>
> 4) you include a stack-trace (thanks for that) but things have been 
> changed, get_situation_map is gone from enrich_intersection_points, 
> and so might be the whole problem. This is by chance released this week
> So my new question is: does it work for you in 1.57?
>
> We moved our entire project to 1.57.0 (which is part of the reason why 
> I did not reply earlier). The apparently good news is: I cannot 
> reproduce the crash any more. I even made sure, that 
> copy_segment_pointdoes not return false, and in my quick testing so 
> far, it never did, which (to my understanding) means that 
> point_outactually gets initialized.
>
> I still find it suspicious that consider_relative_ordercalls 
> copy_segment_pointswithout checking for the return value. If 
> copy_segment_pointand consequently copy_segment_pointswould return 
> false, then pi, pj, ri, rj, si, sjwould be uninitialized and all kinds 
> of things could happen further down. I would feel much better if at 
> that point there were an assertion, or an exception thrown, because 
> then the problem -- if there is any -- would become obvious 
> independent of the actual values that happen to be found in 
> uninitialized memory.
>

Yes you are right. Actually I already did, because of your last mail. 
After inspecting the code I made a fix. For open polygons, copying the 
very last segment, it did not work correctly and therefore failed. This 
was because the size of the range was inspected before the view. Most 
probably this was the problem. That was after 1.57 was released, so you 
don't have it. It's this commit:

https://github.com/boostorg/geometry/commit/16fb68921763ca188397d5e1bebf6ff005ecdaa8

In fact I implemented your suggestion too, because it asserts now and 
always return true. We can remove its return value now (I think I will do).

> Thus here is my suggestion: Since this piece of code is vulnerable to 
> problems that lead to errors much later, and those errors are 
> inherently hard to reproduce, you should assert or throw whenever 
> copy_segment_pointsdoes not initialize its out-parameters. With this 
> kind of safeguard in place, the issue could be closed for the time 
> being. If there is still a problem in 1.57.0, or if new problems are 
> introduced in later versions, we will then learn soon enough.
>
> https://svn.boost.org/trac/boost/ticket/10719
>

So reading this, I could indeed close the ticket now..

Thanks again for your message.

Regards, Barend