Opened 9 years ago

Closed 9 years ago

#6244 closed bug (fixed)

BTwoDImensionalLayout::AlignWith() segfaults

Reported by: yourpalal Owned by: axeld
Priority: normal Milestone: R1
Component: Kits/Interface Kit Version: R1/Development
Keywords: TwoDimensionalLayout Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

Currently, the BTwoDimensionalLayout::AlignWith() method segfaults. This is due to out of bounds access of a BList (which returns NULL). The only place this method is ever called is in LayoutTest1 (according to OpenGrok), so this has gone undetected for quite a while, I think.

Attachments (1)

alignWith.patch (820 bytes ) - added by yourpalal 9 years ago.
Patch to fix segfault, also replaces an old style cast with a static_cast.

Download all attachments as: .zip

Change History (5)

by yourpalal, 9 years ago

Attachment: alignWith.patch added

Patch to fix segfault, also replaces an old style cast with a static_cast.

comment:1 by yourpalal, 9 years ago

Has a Patch: set

comment:3 by bonefish, 9 years ago

Why that would fix anything isn't obvious to me. Can you explain, please.

in reply to:  3 comment:4 by yourpalal, 9 years ago

Replying to bonefish:

Why that would fix anything isn't obvious to me. Can you explain, please.

Certainly! The problem is that with the original code, the for loop moves forwards through the list, 0,1,2,3... n, but for each local layouter calls SetCompoundLayouter. LocalLayouter::SetCompoundLayouter() calls CompoundLayouter::RemoveLocalLayouter(this) on the compound layouter that is being replaced, which then removes this LocalLayouter from its fLocalLayouters BList. The problem is that when you remove an item from index 0, all the other items get shifted down an index, so now the LocalLayouter that had previously been at index n is now at index n-1. There is nothing at index n, so the BList will return NULL when you request something at index n. Another solution is to always work at index 0, and use a while (other->fLocalLayouters.CountItems() > 0). Its a bit hard to see at first because of the different levels of abstraction in the AbsorbCompoundLayouters() method.

comment:5 by pulkomandy, 9 years ago

Resolution: fixed
Status: newclosed

Applied in hrev37348.

Note: See TracTickets for help on using tickets.