On 3-8-2013 23:38, Jon Cruz wrote:

On Aug 3, 2013, at 12:43 PM, Johan Engelen wrote:

...

Hi all, I just came across this piece of code: memset(style, 0, sizeof(SPStyle)); That looks terrifying! SPStyle is a huge struct, containing member objects (sigc::connection) besides just simple integers and enums. CPPCheck complains with "Using 'memset' on struct that contains a virtual method.". I can't find any virtual methods in SPStyle, but still it looks like a bad bug.

I have disabled that line of code, and added extra re-init lines in r12452. Please have a look and confirm the fix.

Good catch. The memset is definitely wrong, but I'll have to look a bit more to figure out the correct fix. I'm thinking that some of the code spread around should end up centralized.

*However* I wanted to point out that there is a problem with how memset is used there. Aside from the high-level issue that it should not be used at all in this case, the manner in which it was applied is unsafe.

When calling memset, one should never use a sizeof(struct_type), but instead use sizeof(variable). In this case it should generally be

 memset(style, 0, sizeof(*style));

or memset(&style, 0, sizeof(style));

Using a pointer (the first case) is additionally dangerous as one really should take in a parameter for the size instead of using a hardcoded value.

Luckily we only should end up with memset for structs that are used by C system calls, and can purge its use elsewhere.

The reason I found it is that cppcheck complained about the virtual method thing. I always thought in case of virtual methods, the pointer "style" points to the VTable (or Vtable pointer) first, and the actual struct members start after that. So in that case, memset will overwrite the vtable entry, right?

Bad bad bad! Cheers, Johan