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

[MC Return]

Added several field width specifiers to prevent overflows with massive input data.

Adds several field width specifiers to prevent overflows with massive input data.

[Daniel van Vugt (vanvugt)]

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.

[Daniel van Vugt (vanvugt)]

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.

review: Needs Fixing

[MC Return (mc-return)]

> 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.

[MC Return (mc-return)]

> 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 ?

[MC Return (mc-return)]

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.

[Daniel van Vugt (vanvugt)]

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.

[MC Return (mc-return)]

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 ?

[MC Return (mc-return)]

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

[Daniel van Vugt (vanvugt)]

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).

[Daniel van Vugt (vanvugt)]

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

[MC Return (mc-return)]

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.

[MC Return (mc-return)]

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...

[MC Return (mc-return)]

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...

[Daniel van Vugt (vanvugt)]

Please STOP PASTING

[Daniel van Vugt (vanvugt)]

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

[MC Return (mc-return)]

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.

[MC Return (mc-return)]

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

[Daniel van Vugt (vanvugt)]

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

review: Disapprove

[MC Return (mc-return)]

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.

Unmerged revisions

3312. By MC Return on 2012-08-06

Changed the int buttonNum to unsigned int buttonNum and changed "%d" to "%2u", because we do not have negative buttonNums and never more than 99 buttons

3311. By MC Return on 2012-08-06

Added field width specifiers (%5d x2) to "screenshot%d"

3310. By MC Return on 2012-08-06

Added field width specifier " %256s " to sscanf (for variable nameTrimmed)

3309. By MC Return on 2012-08-06

Added field width specifiers to " %d " changing them to " %5d " for the int variables vb and vi

3308. By MC Return on 2012-08-06

Added a field width specifier to " %s " changing it to " %256s " to allow a maximum of 256 characters for optNamesValues (is it too much ?)

3307. By MC Return on 2012-08-06

Changed 'int buttonNum;' to 'unsigned int buttonNum;', because buttonNum should always be positive
Changed 'if (sscanf (binding + strlen ("Button"), "%d", &buttonNum) == 1)' to 'if (sscanf (binding + strlen ("Button"), "%2u", &buttonNum) == 1)', because buttonNum should never be higher than 20

3306. By MC Return on 2012-08-06

Changed 'sscanf (token, "screen%d", &screenNum);' to 'sscanf (token, "screen%5u", &screenNum);', adding a field width specifier and changing the type from decimal integer to an unsigned integer (min 0, max 65536)

3305. By MC Return on 2012-08-06

Prevent overflow of name[1024] by adding a field width specifier (%1023s)

=== modified file 'compizconfig/gconf/src/gconf.c'
--- compizconfig/gconf/src/gconf.c	2012-07-03 06:22:53 +0000
+++ compizconfig/gconf/src/gconf.c	2012-08-06 18:15:23 +0000
@@ -476,7 +476,7 @@
     if (!token)
 	return;
 
-    sscanf (token, "screen%d", &screenNum);
+    sscanf (token, "screen%5u", &screenNum);
 
     token = strsep (&keyName, "/"); /* 'options' */
     if (!token)

=== modified file 'compizconfig/libcompizconfig/src/bindings.c'
--- compizconfig/libcompizconfig/src/bindings.c	2012-07-29 10:11:30 +0000
+++ compizconfig/libcompizconfig/src/bindings.c	2012-08-06 18:15:23 +0000
@@ -375,9 +375,9 @@
 
     if (strncmp (binding, "Button", strlen ("Button")) == 0)
     {
-	int buttonNum;
+	unsigned int buttonNum;
 
-	if (sscanf (binding + strlen ("Button"), "%d", &buttonNum) == 1)
+	if (sscanf (binding + strlen ("Button"), "%2u", &buttonNum) == 1)
 	{
 	    value->button        = buttonNum;
 	    value->buttonModMask = mods;

=== modified file 'compizconfig/libcompizconfig/src/compiz.cpp'
--- compizconfig/libcompizconfig/src/compiz.cpp	2012-07-30 11:16:25 +0000
+++ compizconfig/libcompizconfig/src/compiz.cpp	2012-08-06 18:15:23 +0000
@@ -3036,7 +3036,7 @@
     for (i = 0; i < nFile; i++)
     {
 	char name[1024];
-	sscanf (nameList[i]->d_name, "lib%s", name);
+	sscanf (nameList[i]->d_name, "lib%1023s", name);
 	if (strlen (name) > 3)
 	    name[strlen (name) - 3] = 0;
 	free (nameList[i]);

=== modified file 'plugins/animation/src/options.cpp'
--- plugins/animation/src/options.cpp	2010-10-23 15:41:33 +0000
+++ plugins/animation/src/options.cpp	2012-08-06 18:15:23 +0000
@@ -124,7 +124,7 @@
     char *optNamesValues = (char *)calloc (len + 1, 1);
 
     // Find the first substring with no spaces in it
-    sscanf (optNamesValuesOrig, " %s ", optNamesValues);
+    sscanf (optNamesValuesOrig, " %256s ", optNamesValues);
     if (!strlen (optNamesValues))
     {
 	free (optNamesValues);
@@ -166,7 +166,7 @@
 	    break;
 	}
 
-	sscanf (name, " %s ", nameTrimmed);
+	sscanf (name, " %256s ", nameTrimmed);
 	if (!strlen (nameTrimmed))
 	{
 	    errorNo = 2;
@@ -221,14 +221,14 @@
 	{
 	case CompOption::TypeBool:
 	    int vb;
-	    valueRead = sscanf (valueStr, " %d ", &vb);
+	    valueRead = sscanf (valueStr, " %5d ", &vb);
 	    if (valueRead)
 		pair->value.set ((bool)vb);
 	    break;
 	case CompOption::TypeInt:
 	{
 	    int vi;
-	    valueRead = sscanf (valueStr, " %d ", &vi);
+	    valueRead = sscanf (valueStr, " %5d ", &vi);
 	    if (valueRead > 0)
 	    {
 		if (o->rest ().inRange (vi))

=== modified file 'plugins/screenshot/src/screenshot.cpp'
--- plugins/screenshot/src/screenshot.cpp	2012-01-18 16:26:45 +0000
+++ plugins/screenshot/src/screenshot.cpp	2012-08-06 18:15:23 +0000
@@ -122,7 +122,7 @@
 {
     int number;
 
-    if (sscanf (d->d_name, "screenshot%d.png", &number))
+    if (sscanf (d->d_name, "screenshot%5d.png", &number))
     {
 	int nDigits = 0;
 
@@ -201,7 +201,7 @@
 
 			if (n > 0)
 			    sscanf (namelist[n - 1]->d_name,
-				    "screenshot%d.png",
+				    "screenshot%5d.png",
 				    &number);
 
 			number++;

=== modified file 'src/action.cpp'
--- src/action.cpp	2012-05-10 12:58:01 +0000
+++ src/action.cpp	2012-08-06 18:15:23 +0000
@@ -287,9 +287,9 @@
 
     if (start != str.size () && str.compare (start, 6, "Button") == 0)
     {
-	int buttonNum;
+	unsigned int buttonNum;
 
-	if (sscanf (str.substr (start + 6).c_str (), "%d", &buttonNum) == 1)
+	if (sscanf (str.substr (start + 6).c_str (), "%2u", &buttonNum) == 1)
 	{
 	    mButton    = buttonNum;
 	    mModifiers = mods;