EEschema crash on block paste

[Andrew Dyer]

kicad build (2010-05-05 BZR 2356)-stable running under Windows 7, 2Gb memory, AMD Phenom 8450, ATI radeon HD 3200 graphics.

I had created a one page schematic which started to get crowded, so I added a connectionless hierarchy symbol on the first page with the intent of moving some parts of the schematic onto the new page to free up some room. I can visit the new page OK and I set up the page size to 'B' and copied over the title block information by editing the master copy on the first page and exporting the page settings. Next I select the block components to be moved, right click and select 'save block'. next I navigate into the new page using the hierarchy navigator, click the paste icon, I see the component outlines, I move them to where I want, click to paste them in place and eeschema crashes and I see the standard windows dialog box for a crashed program.

I tried doing the same operation on a much smaller block (one 4 pin connector, a few nets and a ground symbol) and that worked correctly with no crash.

[Andreas Ortmann (elektron7)]

I have the same build of kicad running under Gentoo (sci-electronics/kicad-20100505_p2356), AMD Athlon 64 4600+, ATI Radeon X1650pro, 1GB RAM.

After doing almost the same thing, I experience this crash too.

I click on 'Place hierarchical sheet' and place a sheet inside my full one page schematic and give it a name in the dialog. However, I think I can isolate the cause of the crash: It seems to depend on what I select via 'save block' because the crash ONLY occurs if I include the sheet component in the selection (and try pasting the stuff later after entering the sheet).

[Martin Errenst (imp-d)]

knoc! I knew it - you are playing around with sheet loops again :D. I remember you've told be about this in person :).

I can reproduce this bug with r4714. Looking at the backtrace, it's stuck in a loop until it explodes.

Guess the fix will be a check before saving, that there are no loops inside the tree of sheets.
Checking if the parent sheet is the same as the current sheet will not help if the loop is bigger. Also checking if a sheet is referenced multiple times doesn't work, since one can save quite some work by instantiating a sheet multiple times.

[Nick Østergaard (nickoe)]

Please retest if this can still be reproduced with latest product?

Please provide exact and easy to follow steps to reproduce or at least an example schematic?

[Brian Sidebotham (brian-sidebotham)]

I can confirm this bug - easy to repeat:

(1) Start a new project, start EESCHEMA
(2) Confirm to create the first schematic sheet
(3) Add a new hierarchical sheet, named anything
(4) Navigate to hierarchical sheet
(5) Add a new hierarchical sheet, named the same as the root sheet
(6) Confirm to use the file as the sheet, and welcome to SIGSEGV.

Marking this bug as critical because of the confirmed SIGSEGV and will patch it in the next day or so...

[jean-pierre charras (jp-charras)]

Brian,
I am thinking you have created a hierarchical infinite loop in a complex hierarchy, if you have given the same filename to the root sheet and to the included sheet (the sheet includes itself).
Be careful: detecting infinite loops in a complex hierarchy is not easy and could need more than one day.
(complex hierarchies are very tricky cases)

[Wayne Stambaugh (stambaughw)]

The infinite loop is SCH_SHEET::CountSheets(). This is due the the
sheet referencing itself so the DLIST previous and next pointers are the
same. Is there any case where a schematic would have a sheet embedded
in itself makes any sense? It seems to me this should be an illegal
operation. If that is the case, it will also have to be fixed the copy
and block copy functions as well. The new sheet creation code can be
found in sheet.cpp. If it does make sense to embed a sheet within
itself, then all of the sheet hierarchy code will have to be gone over
very carefully.

On 4/23/2015 11:42 AM, jean-pierre charras wrote:
> Brian,
> I am thinking you have created a hierarchical infinite loop in a complex hierarchy, if you have given the same filename to the root sheet and to the included sheet (the sheet includes itself).
> Be careful: detecting infinite loops in a complex hierarchy is not easy and could need more than one day.
> (complex hierarchies are very tricky cases)
>

[jean-pierre charras (jp-charras)]

I also am thinking a sheet embedded in itself makes no sense and this is an illegal operation.
But (I can be wrong) I am thinking this case is no so easy to detect (although detect it is certainly feasible).

[Wayne Stambaugh (stambaughw)]

You could set the parent of every SCH_ITEM to the sheet that contains it
(only the root SCH_SHEET would have no parent), this way assigning the
parent of a SCH_SHEET to itself would be easy to detect.

if( item->Type() == SCH_SHEET && item->GetParent() == aParentSheet )
    FAIL!!!!

The more I think about it, the more this seems like an invalid
operation. I just checked my old version of Orcad Captuire(10.0i) and I
found this ominous warning in the help:

"Be careful not to create recursion in your design. Capture cannot
prevent recursion, and the Design Rules Check command does not report it.

Recursion causes Capture to process infinitely as it tries to expand the
design, resulting in the loss of any changes you've made to your design
since it was last saved."

We are not the first schematic capture program to struggle with this issue.

On 4/23/2015 2:34 PM, jean-pierre charras wrote:
> I also am thinking a sheet embedded in itself makes no sense and this is an illegal operation.
> But (I can be wrong) I am thinking this case is no so easy to detect (although detect it is certainly feasible).
>

[jean-pierre charras (jp-charras)]

A sheet inside itself is a very basic case.

When we are using already a complex hierarchy, where the same sheet is already used more than once, there are a lot of other (tricky) cases.

I am not surprised Orcad is not able to detect a recursion: this is really tricky is some cases.

Perhaps (I did not tried that) one could build the sheet paths, and if the number of sheets in a path being built reaches a limit (say 1000 for instance ) a recursion is certainly found in this path.

[Wayne Stambaugh (stambaughw)]

I think I have a complete solution. The current SCH_SHEET_PATH is the
key. A SCH_SHEET file cannot be added to an existing SCH_SHEET_PATH if
the same SCH_SHEET file is already defined within that path. Any
attempt to do this will cause recursion.

root_sheet/sheet1/sheet2/sheet3/.../sheetn // LEGAL

root_sheet/sheet1/sheet1 // ILLEGAL

root_sheet/sheet1/sheet2/sheet1 // ILLEGAL

root_sheet/sheet1/sheet2/sheet3 // LEGAL
      +--> sheet1/sheet3/sheet2
      +--> sheet2/sheet1/sheet3
      +--> sheet3/sheet2/sheet1

Checking for the existence of a new SCH_SHEET file within the current
SCH_SHEET_PATH should work in every instance including the copy/paste
issue. It will require some careful coding but this should be a robust
solution.

On 4/24/2015 6:05 AM, jean-pierre charras wrote:
> A sheet inside itself is a very basic case.
>
> When we are using already a complex hierarchy, where the same sheet is
> already used more than once, there are a lot of other (tricky) cases.
>
> I am not surprised Orcad is not able to detect a recursion: this is
> really tricky is some cases.
>
> Perhaps (I did not tried that) one could build the sheet paths, and if
> the number of sheets in a path being built reaches a limit (say 1000 for
> instance ) a recursion is certainly found in this path.
>

[Brian Sidebotham (brian-sidebotham)]

Checking the path seems reasonable, I agree. It looks like the most
sane way to test for a recursion problem.

CountSheets could also have a maximum recursion depth too in order not
to explode with a SIGSEGV and instead return an error code. Something
a bit more defensive than what is there now will help at least.

Thanks for all the discussion about this, sorry I missed it, it's helped a lot!

On 24 April 2015 at 14:55, Wayne Stambaugh <email address hidden> wrote:
> I think I have a complete solution. The current SCH_SHEET_PATH is the
> key. A SCH_SHEET file cannot be added to an existing SCH_SHEET_PATH if
> the same SCH_SHEET file is already defined within that path. Any
> attempt to do this will cause recursion.
>
> root_sheet/sheet1/sheet2/sheet3/.../sheetn // LEGAL
>
> root_sheet/sheet1/sheet1 // ILLEGAL
>
> root_sheet/sheet1/sheet2/sheet1 // ILLEGAL
>
> root_sheet/sheet1/sheet2/sheet3 // LEGAL
> +--> sheet1/sheet3/sheet2
> +--> sheet2/sheet1/sheet3
> +--> sheet3/sheet2/sheet1
>
> Checking for the existence of a new SCH_SHEET file within the current
> SCH_SHEET_PATH should work in every instance including the copy/paste
> issue. It will require some careful coding but this should be a robust
> solution.
>
>
> On 4/24/2015 6:05 AM, jean-pierre charras wrote:
>> A sheet inside itself is a very basic case.
>>
>> When we are using already a complex hierarchy, where the same sheet is
>> already used more than once, there are a lot of other (tricky) cases.
>>
>> I am not surprised Orcad is not able to detect a recursion: this is
>> really tricky is some cases.
>>
>> Perhaps (I did not tried that) one could build the sheet paths, and if
>> the number of sheets in a path being built reaches a limit (say 1000 for
>> instance ) a recursion is certainly found in this path.
>>
>
> --
> You received this bug notification because you are a bug assignee.
> Matching subscriptions: kicad-bugs-subs
> https://bugs.launchpad.net/bugs/689444
>
> Title:
> EEschema crash on block paste
>
> Status in KiCad EDA Software Suite:
> Confirmed
>
> Bug description:
> kicad build (2010-05-05 BZR 2356)-stable running under Windows 7, 2Gb
> memory, AMD Phenom 8450, ATI radeon HD 3200 graphics.
>
> I had created a one page schematic which started to get crowded, so I
> added a connectionless hierarchy symbol on the first page with the
> intent of moving some parts of the schematic onto the new page to free
> up some room. I can visit the new page OK and I set up the page size
> to 'B' and copied over the title block information by editing the
> master copy on the first page and exporting the page settings. Next I
> select the block components to be moved, right click and select 'save
> block'. next I navigate into the new page using the hierarchy
> navigator, click the paste icon, I see the component outlines, I move
> them to where I want, click to paste them in place and eeschema
> crashes and I see the standard windows dialog box for a crashed
> program.
>
> I tried doing the same operation on a much smaller block (one 4 pin
> connector, a few nets an...

[Wayne Stambaugh (stambaughw)]

On 4/24/2015 10:28 AM, Brian Sidebotham wrote:
> Checking the path seems reasonable, I agree. It looks like the most
> sane way to test for a recursion problem.

The easiest thing I can think of is add a HasFileName( const wxString&
aFileName ) function to SCH_SHEET_PATH that checks every sheet in the
SCH_SHEET_PATH for aFileName. You'll have to be careful with the
absolute file path versus relative file path issue with the file name
comparison. This test will be fairly straight forward when adding new
sheets or modifying existing sheets. The tricky part will be when you
paste a sheet or a block with multiple sheets. Every pasted sheet path
will have to be tested against the current sheet path to ensure no
recursion can occur. If you get stuck or don't have time to fix this, I
may have some time to look at it this weekend.

>
> CountSheets could also have a maximum recursion depth too in order not
> to explode with a SIGSEGV and instead return an error code. Something
> a bit more defensive than what is there now will help at least.

I don't really like this solution because it masks the real problem.
Adding an assertion as warning to developers that something is broken is
OK as far as defensive programming goes but blindly creating a sheet
hierarchy with some arbitrary depth is not what the user will expect.
My guess is even if you limit the recursion depth, when you go perform
any function on the hierarchy (annotate, netlist, save, etc.), it will
explode because how do you link a sheet to itself without creating other
issues?

>
> Thanks for all the discussion about this, sorry I missed it, it's helped
> a lot!
>
> On 24 April 2015 at 14:55, Wayne Stambaugh <email address hidden> wrote:
>> I think I have a complete solution. The current SCH_SHEET_PATH is the
>> key. A SCH_SHEET file cannot be added to an existing SCH_SHEET_PATH if
>> the same SCH_SHEET file is already defined within that path. Any
>> attempt to do this will cause recursion.
>>
>> root_sheet/sheet1/sheet2/sheet3/.../sheetn // LEGAL
>>
>> root_sheet/sheet1/sheet1 // ILLEGAL
>>
>> root_sheet/sheet1/sheet2/sheet1 // ILLEGAL
>>
>> root_sheet/sheet1/sheet2/sheet3 // LEGAL
>> +--> sheet1/sheet3/sheet2
>> +--> sheet2/sheet1/sheet3
>> +--> sheet3/sheet2/sheet1
>>
>> Checking for the existence of a new SCH_SHEET file within the current
>> SCH_SHEET_PATH should work in every instance including the copy/paste
>> issue. It will require some careful coding but this should be a robust
>> solution.
>>
>>
>> On 4/24/2015 6:05 AM, jean-pierre charras wrote:
>>> A sheet inside itself is a very basic case.
>>>
>>> When we are using already a complex hierarchy, where the same sheet is
>>> already used more than once, there are a lot of other (tricky) cases.
>>>
>>> I am not surprised Orcad is not able to detect a recursion: this is
>>> really tricky is some cases.
>>>
>>> Perhaps (I did not tried that) one could build the sheet paths, and if
>>> the number of sheets in a path being built reaches a limit (say 1000 for
>>> instance ) a recursion is certainly found in this path.
>>>
>>
>> --
>> You received thi...

[jean-pierre charras (jp-charras)]

IWayne,
I am not sure i understant your example:
root_sheet/sheet1/sheet2/sheet3
      +--> sheet1/sheet3/sheet2
      +--> sheet2/sheet1/sheet3
      +--> sheet3/sheet2/sheet1
Do you mean your example is a complex hierarchy, with 4 sheets visible inside the root sheet?

If yes, I am thinking the case
root_sheet/sheet1/sheet2/sheet3
      +--> sheet1/sheet3/sheet2
is a lot of recursion, because there are a lot of other paths in this "design"

In your example, the first path is
root_sheet/sheet1/sheet2/sheet3
the second path is
root_sheet/sheet1/sheet3/sheet2

But if your design includes root_sheet/sheet1/sheet2/sheet3, it imply the fact sheet1 always contains sheet2 and sheet2 contains always sheet3.
(You have one only one schematic for sheet1, sheet2 and sheet3)
if the second path exists: root_sheet/sheet1/sheet3/sheet2 it imply many other paths:
for instance :
root_sheet/sheet1/sheet3/sheet2/sheet3 (because sheet2 contains sheet3, when you have designed your first schematic path)

This kind of case happens easily in copy/paste block (which creates currently a serious risk, if it includes a sheet, because it can include a lot of sub-sheets and creates complex hierarchies and recursion).

This is also why when importing sheets (in import sheet command), the sub-sheets are not imported.

[Wayne Stambaugh (stambaughw)]

You are correct. I missed that case. This means that you will have to
test the entire hierarchy against any added hierarchy to make sure that
no recursion occurs. The means that a function CheckForRecusion(
SCH_SHEET_PATH* aSheetPath ) would have to be added to SCH_SHEET_PATH.
For new sheets, the SCH_SHEET_PATH would have to be created from the
.sch file and tested against the full hierarchy for potential recursion.
 Any recursion should be rejected.

On 4/24/2015 11:40 AM, jean-pierre charras wrote:
> IWayne,
> I am not sure i understant your example:
> root_sheet/sheet1/sheet2/sheet3
> +--> sheet1/sheet3/sheet2
> +--> sheet2/sheet1/sheet3
> +--> sheet3/sheet2/sheet1
> Do you mean your example is a complex hierarchy, with 4 sheets visible inside the root sheet?
>
> If yes, I am thinking the case
> root_sheet/sheet1/sheet2/sheet3
> +--> sheet1/sheet3/sheet2
> is a lot of recursion, because there are a lot of other paths in this "design"
>
> In your example, the first path is
> root_sheet/sheet1/sheet2/sheet3
> the second path is
> root_sheet/sheet1/sheet3/sheet2
>
> But if your design includes root_sheet/sheet1/sheet2/sheet3, it imply the fact sheet1 always contains sheet2 and sheet2 contains always sheet3.
> (You have one only one schematic for sheet1, sheet2 and sheet3)
> if the second path exists: root_sheet/sheet1/sheet3/sheet2 it imply many other paths:
> for instance :
> root_sheet/sheet1/sheet3/sheet2/sheet3 (because sheet2 contains sheet3, when you have designed your first schematic path)
>
> This kind of case happens easily in copy/paste block (which creates
> currently a serious risk, if it includes a sheet, because it can include
> a lot of sub-sheets and creates complex hierarchies and recursion).
>
> This is also why when importing sheets (in import sheet command), the
> sub-sheets are not imported.
>

[Wayne Stambaugh (stambaughw)]

Brian and JP,

Are either you working on this issue? I haven't heard anything so I
didn't want to do any work on it myself. If not, please let me know and
I will see if I can get it resolved. This one definitely needs to be
resolved before the stable release.

Thanks,

Wayne

On 4/24/2015 11:40 AM, jean-pierre charras wrote:
> IWayne,
> I am not sure i understant your example:
> root_sheet/sheet1/sheet2/sheet3
> +--> sheet1/sheet3/sheet2
> +--> sheet2/sheet1/sheet3
> +--> sheet3/sheet2/sheet1
> Do you mean your example is a complex hierarchy, with 4 sheets visible inside the root sheet?
>
> If yes, I am thinking the case
> root_sheet/sheet1/sheet2/sheet3
> +--> sheet1/sheet3/sheet2
> is a lot of recursion, because there are a lot of other paths in this "design"
>
> In your example, the first path is
> root_sheet/sheet1/sheet2/sheet3
> the second path is
> root_sheet/sheet1/sheet3/sheet2
>
> But if your design includes root_sheet/sheet1/sheet2/sheet3, it imply the fact sheet1 always contains sheet2 and sheet2 contains always sheet3.
> (You have one only one schematic for sheet1, sheet2 and sheet3)
> if the second path exists: root_sheet/sheet1/sheet3/sheet2 it imply many other paths:
> for instance :
> root_sheet/sheet1/sheet3/sheet2/sheet3 (because sheet2 contains sheet3, when you have designed your first schematic path)
>
> This kind of case happens easily in copy/paste block (which creates
> currently a serious risk, if it includes a sheet, because it can include
> a lot of sub-sheets and creates complex hierarchies and recursion).
>
> This is also why when importing sheets (in import sheet command), the
> sub-sheets are not imported.
>

[jean-pierre charras (jp-charras)]

No, I am not working on it.

[Wayne Stambaugh (stambaughw)]

Finally got around to fixing the recursion bug that caused this segfault. I fixed it not only in the block paste but also the edit sheet code so it should be fixed everywhere that the sheet paths can be changed in Eeschema. I committed the change in the product branch r5270. Please test it thoroughly to see if there are any corner cases I missed.