Review Request: Fix KPixmapCache crash (probably because of pointer aliasing in unions)
Johannes Sixt
j.sixt at viscovery.net
Mon May 25 08:55:37 BST 2009
Michael Pyne schrieb:
> On Sunday 24 May 2009 20:09:23 Christoph Feck wrote:
>> Attempts to fix KPixmapCrash, pardon, KPixmapCache. This is my first
>> submission to reviewboard to learn how it works (and I already learned that
>> you have to retype the Description???).
>>
>> According to the comments in the (previous) code, there are problems using
>> the pointers in the union, so I removed them, and replaced them with a
>> single pointer and casts at the appropriate places.
>>
>>
>> This addresses bug 182026.
>> https://bugs.kde.org/show_bug.cgi?id=182026
>
> Well I lost my KWallet and thence my super-secure review board password.
> Luckily for me there is absolutely no way I can find to reset my password so
> I'll resort to good-old-fashioned mailing lists.
>
> I wrote the code in question, including the immortal comment "This probably
> breaks some C++ aliasing rule :-("
>
> Turns out I was right =D
>
> I had actually after writing the comment got myself under the impression that
> the char* memory in the union made type-punning OK but apparently that doesn't
> allow you to actually ever dereference the pointer and expect it to work.
> Weird.
>
> I say ship it
No, don't ship it. The new code is just as wrong as the old code was.
The new code uses the idiom
*(some_type*)foo
where foo is of some_other_type. This breaks aliasing rules. The old code
only hid the type cast in a union:
union bla {
char* memory;
some_type* a;
some_other_type* b;
};
The correct solution is to change the union to
union bla {
some_type a;
some_other_type b;
};
bla* memory;
and use that appropriately.
-- Hannes
--
"Atomic objects are neither active nor radioactive." --
Doc.No N2798 (Working Draft, Standard for Programming Language C++)
More information about the kde-core-devel
mailing list