Merge lp://qastaging/~mc-return/compiz/compiz.fix1030473-part4 into lp://qastaging/compiz/0.9.8

Out of curiosity, how many parts will there be? It's very hard to mark a bug as fixed when I'm not sure where the end of the fix will be.

The integer changes (%d and %u) are low value. An integer overflow is different to a string overrun. Adding a field size to %d does not really improve the robustness of the code unless you have very good error checking:
    if (sscanf(...) == N) {} else { ... }

Also, field size limits are pointless and dangerous if we never check the return value of sscanf. Adding the field sizes means sscanf might fail if the input is too big. However if the surrounding code does not check sscanf for errors then you will get uninitialized variables, which is worse.

> Out of curiosity, how many parts will there be? It's very hard to mark a bug
> as fixed when I'm not sure where the end of the fix will be.

Hmm, we have already reduced the remaining issues from a 108KiB text file to under 30KiB and I have been updating and documenting remaining problems in the bug report.
We could ofc. make this the last part if you like and fix the rest of the problems here.

> The integer changes (%d and %u) are low value. An integer overflow is
> different to a string overrun. Adding a field size to %d does not really
> improve the robustness of the code unless you have very good error checking:
> if (sscanf(...) == N) {} else { ... }
>
> Also, field size limits are pointless and dangerous if we never check the
> return value of sscanf. Adding the field sizes means sscanf might fail if the
> input is too big. However if the surrounding code does not check sscanf for
> errors then you will get uninitialized variables, which is worse.

Should I simply remove the field width specifiers from the integers or add additional error checking ?

Sam told me on Sunday he will take care about those 2 confirmed problems:

compizconfig/gsettings/src/gsettings.c:296: (warning) Casting between double* and float* which have an incompatible binary data representation

compizconfig/libcompizconfig/src/main.c:4305: (error) Read and write operations without a call to a positioning function (fseek, fsetpos or rewind) or fflush inbetween result in undefined behaviour.

Please just remove the field width for integers. But you still also need error checking around the other sscanf() calls for strings with a max field width.

Then we have possible null pointer dereferences:

compizconfig/libcompizconfig/src/main.c:656]: (error) Possible null pointer dereference: subGroup - otherwise it is redundant to check if subGroup is null at line 657
compizconfig/libcompizconfig/src/main.c:3213]: (error) Possible null pointer dereference: conflict - otherwise it is redundant to check if conflict is null at line 3214

gtk/window-decorator/frames.c:266]: (error) Possible null pointer dereference: frame - otherwise it is redundant to check if frame is null at line 296
gtk/window-decorator/frames.c:269]: (error) Possible null pointer dereference: frame - otherwise it is redundant to check if frame is null at line 296
gtk/window-decorator/frames.c:272]: (error) Possible null pointer dereference: frame - otherwise it is redundant to check if frame is null at line 296
gtk/window-decorator/frames.c:275]: (error) Possible null pointer dereference: frame - otherwise it is redundant to check if frame is null at line 296

gtk/window-decorator/frames.c:278]: (error) Possible null pointer dereference: frame - otherwise it is redundant to check if frame is null at line 296
gtk/window-decorator/frames.c:281]: (error) Possible null pointer dereference: frame - otherwise it is redundant to check if frame is null at line 296
gtk/window-decorator/frames.c:284]: (error) Possible null pointer dereference: frame - otherwise it is redundant to check if frame is null at line 296
gtk/window-decorator/frames.c:287]: (error) Possible null pointer dereference: frame - otherwise it is redundant to check if frame is null at line 296
gtk/window-decorator/frames.c:290]: (error) Possible null pointer dereference: frame - otherwise it is redundant to check if frame is null at line 296
gtk/window-decorator/frames.c:293]: (error) Possible null pointer dereference: frame - otherwise it is redundant to check if frame is null at line 296

Should I remove the redundant null checks ?

Common realloc mistakes:

libcompizconfig/backend/src/ini.c:187]: (error) Common realloc mistake: 'privData' nulled but not freed upon failure
libcompizconfig/backend/src/ini.c:221]: (error) Common realloc mistake: 'privData' nulled but not freed upon failure

libcompizconfig/src/filewatch.c:113]: (error) Common realloc mistake: 'fwData' nulled but not freed upon failure
libcompizconfig/src/filewatch.c:173]: (error) Common realloc mistake: 'fwData' nulled but not freed upon failure

libcompizconfig/src/ini.c:791]: (error) Common realloc mistake: 'stringBuffer' nulled but not freed upon failure

plugins/shift/src/shift.cpp:826]: (error) Common realloc mistake: 'mWindows' nulled but not freed upon failure
plugins/shift/src/shift.cpp:836]: (error) Common realloc mistake: 'mDrawSlots' nulled but not freed upon failure

plugins/stackswitch/src/stackswitch.cpp:619]: (error) Common realloc mistake: 'mWindows' nulled but not freed upon failure
plugins/stackswitch/src/stackswitch.cpp:624]: (error) Common realloc mistake: 'mDrawSlots' nulled but not freed upon failure

Please stop pasting new error details here. Each type of error should be dealt with as a new proposal (and optionally new bug for each).

No, the "redundant" checks probably are not redundant, but need to be moved to an earlier line of code.

Suspicious stuff that can be clarified with parentheses:

animationaddon/src/polygon.cpp:459]: (style) Suspicious calculation. Please use parentheses to clarify the code. The code 'a%b?c:d' should be written as either '(a%b)?c:d' or 'a%(b?c:d)'.

plugins/colorfilter/src/parser.cpp:177]: (style) Suspicious condition (assignment+comparison), it can be clarified with parentheses

plugins/colorfilter/src/parser.cpp:178]: (style) Suspicious condition (assignment+comparison), it can be clarified with parentheses

plugins/place/src/place.cpp:439]: (style) Suspicious expression. Boolean result is used in bitwise operation. The ! operator and the comparison operators have higher precedence than bitwise operators. It is recommended that the expression is clarified with parentheses.

Member variables that are not initialized in the constructors:

plugins/animation/src/glide.cpp:41]: (warning) Member variable 'GlideAnim::glideModRotAngle' is not initialized in the constructor.

plugins/animation/src/magiclamp.cpp:55]: (warning) Member variable 'MagicLampAnim::mTopLeftCornerObject' is not initialized in the constructor.
plugins/animation/src/magiclamp.cpp:55]: (warning) Member variable 'MagicLampAnim::mBottomLeftCornerObject' is not initialized in the constructor.

plugins/animationaddon/src/private.h:41]: (warning) Member variable 'ExtensionPluginAnimAddon::mOutput' is not initialized in the constructor.

plugins/composite/src/screen.cpp:276]: (warning) Member variable 'PrivateCompositeScreen::compositeEvent' is not initialized in the constructor.
plugins/composite/src/screen.cpp:276]: (warning) Member variable 'PrivateCompositeScreen::compositeError' is not initialized in the constructor.
plugins/composite/src/screen.cpp:276]: (warning) Member variable 'PrivateCompositeScreen::compositeOpcode' is not initialized in the constructor.
plugins/composite/src/screen.cpp:276]: (warning) Member variable 'PrivateCompositeScreen::damageEvent' is not initialized in the constructor.
plugins/composite/src/screen.cpp:276]: (warning) Member variable 'PrivateCompositeScreen::damageError' is not initialized in the constructor.
plugins/composite/src/screen.cpp:276]: (warning) Member variable 'PrivateCompositeScreen::fixesEvent' is not initialized in the constructor.
plugins/composite/src/screen.cpp:276]: (warning) Member variable 'PrivateCompositeScreen::fixesError' is not initialized in the constructor.
plugins/composite/src/screen.cpp:276]: (warning) Member variable 'PrivateCompositeScreen::fixesVersion' is not initialized in the constructor.
plugins/composite/src/screen.cpp:276]: (warning) Member variable 'PrivateCompositeScreen::shapeExtension' is not initialized in the constructor.
plugins/composite/src/screen.cpp:276]: (warning) Member variable 'PrivateCompositeScreen::shapeEvent' is not initialized in the constructor.
/plugins/composite/src/screen.cpp:276]: (warning) Member variable 'PrivateCompositeScreen::shapeError' is not initialized in the constructor.
plugins/composite/src/screen.cpp:276]: (warning) Member variable 'PrivateCompositeScreen::randrExtension' is not initialized in the constructor.
plugins/composite/src/screen.cpp:276]: (warning) Member variable 'PrivateCompositeScreen::randrEvent' is not initialized in the constructor.
plugins/composite/src/screen.cpp:276]: (warning) Member variable 'PrivateCompositeScreen::randrError' is not initialized in the constructor.

plugins/ezoom/src/ezoom.cpp:1794]: (warning) Member variable 'CursorTexture::screen' is not initialized in the constructor.
plugins/ezoom/src/ezoom.cpp:1794]: (warning) Member variable 'CursorTexture::width' is not initialized in the constructor.
plugins/ezoom/src/ezoom.cpp:1794]: (warning) Member variable 'CursorTexture::height' is not initialized in the constructor.
plugins/ezoom/src/ezoom.cpp:1794]: (warning) Member variable 'CursorTexture::hotX' is not initialized in the constructor.
plugins/ezoom/src/ezoom.cpp:1794]: (warning) Member variable 'CursorTextu...

C-style pointer castings, that might be intentional:

compizconfig/libcompizconfig/tests/mock-context.h:94]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:100]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:106]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:112]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:118]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:124]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:130]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:136]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:142]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:148]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:154]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:160]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:166]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:172]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:178]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:187]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:196]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:205]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:211]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:217]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:223]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:232]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:241]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:250]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:259]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:265]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:271]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:280]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:289]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:295]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:304]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:313]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:322]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-context.h:331]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/context-mock.cpp:68]: (style) C-style pointer casting
compizconfig/libcompizconfig/tests/mock-plugin.h:64]: (style) C-st...

Please STOP PASTING

Each type of error should be dealt with as a new proposal (and optionally new bug for each).

These might be false positives:

compizconfig/libcompizconfig/backend/src/ini.c:686]: (error) Memory pointed to by 'filePath' is freed twice.

compizconfig/gconf/src/gconf.c:1930]: (error) Memory pointed to by 'pathName' is freed twice.

compizconfig/libcompizconfig/src/main.c:259]: (error) Memory pointed to by 'val' is freed twice.
compizconfig/libcompizconfig/src/main.c:267]: (error) Memory pointed to by 'val' is freed twice.
compizconfig/libcompizconfig/src/main.c:275]: (error) Memory pointed to by 'val' is freed twice.
compizconfig/libcompizconfig/src/main.c:1037]: (error) Memory pointed to by 'dlname' is freed twice.
compizconfig/libcompizconfig/src/main.c:3485]: (error) Memory pointed to by 'backenddir' is freed twice.
compizconfig/libcompizconfig/src/main.c:3495]: (error) Memory pointed to by 'backenddir' is freed twice.

Memory leaks:

compizconfig/libcompizconfig/src/main.c:3626]: (error) Memory leak: sectionName
compizconfig/libcompizconfig/src/main.c:3784]: (error) Memory leak: sectionName
compizconfig/libcompizconfig/src/main.c:4262]: (error) Memory leak: completedUpgrades
compizconfig/libcompizconfig/src/main.c:4266]: (error) Memory leak: completedUpgrades

gtk/window-decorator/decorator.c:1016]: (error) Memory leak: opts

compizconfig/libcompizconfig/src/compiz.cpp:2705]: (error) Memory leak: pbFile
compizconfig/libcompizconfig/src/compiz.cpp:2771]: (error) Memory leak: pbFile

Resource leak:

compizconfig/libcompizconfig/src/main.c:4262]: (error) Resource leak: completedUpgrades

Rejected. Please discuss all of the above errors in NEW and SEPARATE proposals. And resubmit this one as discussed already.

The rest of remaining potential problems that need to be verified:

gtk/window-decorator/metacity.c:599]: (style) Variable 'frame_style' is assigned a value that is never used

plugins/colorfilter/src/parser.h:34]: (style) The class 'FragmentParser 'does not have a constructor but it has attributes. The attributes are not initialized which may cause bugs or undefined behavior.

plugins/group/src/init.cpp:140]: (error) Uninitialized variable: group
plugins/group/src/init.cpp:141]: (error) Uninitialized variable: group

src/screen.cpp:1091]: (error) BOOST_FOREACH caches the end() iterator. It's undefined behavior if you modify the container.

src/window.cpp:2005]: (error) Analysis failed. If the code is valid then please report this failure.