[LEDE-DEV] [PATCH] fstools: Fix some errors detected by cppcheck

[Rosen Penev]

On Mon, Dec 11, 2017 at 1:22 PM, Arjen de Korte <arjen+lede at de-korte.org> wrote:
> Citeren Jonas Gorski <jonas.gorski at gmail.com>:
>
>> On 11 December 2017 at 10:04, Rosen Penev <rosenp at gmail.com> wrote:
>>>
>>> Mainly plugging memory leaks. Size reduction as well. The calloc change
>>> accounts for 272 bytes on this machine for some reason...
>>
>>
>> Please state the exact errors found by cppcheck so we don't have to
>> guess what it found.
>
I used "cppcheck --enable=all --inconclusive --force . 2>err.txt" to
find them. Then I just filtered.

cat err.txt | grep -v never | grep -v reduced

output:

$ cat err.txt | grep -v never | grep -v reduced
[block.c:1087] -> [block.c:1091]: (warning) Either the condition '!pr'
is redundant or there is possible null pointer dereference: pr.
[block.c:1244] -> [block.c:1246]: (style) Variable 'err' is reassigned
a value before the old one has been used.
[block.c:1257] -> [block.c:1259]: (style) Variable 'err' is reassigned
a value before the old one has been used.
[block.c:1330] -> [block.c:1334]: (style) Variable 'err' is reassigned
a value before the old one has been used.
[blockd.c:119]: (warning) Possible null pointer dereference: device
[blockd.c:122]: (warning) Possible null pointer dereference: device
[blockd.c:127]: (warning) Possible null pointer dereference: device
[blockd.c:130]: (warning) Possible null pointer dereference: device
[libblkid-tiny\libblkid-tiny.c:188]: (style) Label 'int' is not used.
[libfstools\overlay.c:290]: (warning) Obsolete function 'alloca'
called. In C99 and later it is recommended to use a variable length
array instead.
[libfstools\overlay.c:314]: (warning) Obsolete function 'alloca'
called. In C99 and later it is recommended to use a variable length
array instead.
[libfstools\rootdisk.c:175]: (error) Resource leak: f
[libfstools\ubi.c:106]: (warning) %u in format string (no. 2) requires
'unsigned int' but the argument type is 'signed int'.
[libfstools\ubi.c:106]: (warning) %u in format string (no. 3) requires
'unsigned int' but the argument type is 'signed int'.
[libfstools\ubi.c:106]: (warning) %u in format string (no. 4) requires
'unsigned int' but the argument type is 'signed int'.
[libfstools\ubi.c:109]: (warning) %u in format string (no. 1) requires
'unsigned int' but the argument type is 'signed int'.
[libfstools\ubi.c:109]: (warning) %u in format string (no. 2) requires
'unsigned int' but the argument type is 'signed int'.
[libfstools\ubi.c:132]: (warning) %u in format string (no. 2) requires
'unsigned int' but the argument type is 'int'.
[libfstools\ubi.c:132]: (warning) %u in format string (no. 3) requires
'unsigned int' but the argument type is 'int'.
[libfstools\ubi.c:132]: (warning) %u in format string (no. 4) requires
'unsigned int' but the argument type is 'int'.
[libfstools\ubi.c:135]: (warning) %u in format string (no. 1) requires
'unsigned int' but the argument type is 'int'.
[libfstools\ubi.c:135]: (warning) %u in format string (no. 2) requires
'unsigned int' but the argument type is 'int'.
[libfstools\ubi.c:72]: (error) Resource leak: f
[libubi\libubi-tiny.c:61]: (portability) 'buf' is of type 'const void
*'. When using void pointers in calculations, the behaviour is
undefined.
[libubi\libubi.c:858]: (warning) Redundant assignment of 'desc' to itself.
[libubi\libubi.c:1026]: (warning) Redundant assignment of 'desc' to itself.
[libubi\libubi.c:1039]: (warning) Redundant assignment of 'desc' to itself.
[libubi\libubi.c:1067]: (warning) Redundant assignment of 'desc' to itself.
[libubi\libubi.c:1095]: (warning) Redundant assignment of 'desc' to itself.
[libubi\libubi.c:1122]: (warning) Redundant assignment of 'desc' to itself.
[libubi\libubi.c:1138]: (warning) Redundant assignment of 'desc' to itself.
[libubi\libubi.c:1148]: (warning) Redundant assignment of 'desc' to itself.
[libubi\libubi.c:985]: (warning) sscanf() without field width limits
can crash with huge input data.
[libubi\libubi.c:1202]: (warning) sscanf() without field width limits
can crash with huge input data.
[libubi\ubiutils-common.c:122]: (style) Redundant condition: If 'bytes
> 1024', the comparison 'bytes != 0' is always true.
(information) Cppcheck cannot find all the include files (use
--check-config for details)

I fixed what I believed was useful, not every warning.

>
> I tried to replicate what Rosen did, but in my case cppcheck came up with
> only two errors:
>
>     [libfstools/rootdisk.c:175]: (error) Resource leak: f
>     [libfstools/ubi.c:72]: (error) Resource leak: f
>
> These look like actual leaks, so this *may* be worth fixing. It could also
> be the conditions never occur (so these never leak anything). The proposed
> changes for these two errors look sane though.
>
>     [block.c:1087] -> [block.c:1091]: (warning) Either the condition '!pr'
> is redundant or there is possible null pointer dereference: pr.
>
> This one has merit too. There is no point in doing a NULL pointer check
> after using that pointer a few lines before.
>
>     [blockd.c:119]: (warning) Possible null pointer dereference: device
>     [blockd.c:127]: (warning) Possible null pointer dereference: device
>     [blockd.c:130]: (warning) Possible null pointer dereference: device
>
> I'm not so sure about these warnings. It doesn't seem possible that *both*
> device_n and device_o are NULL, which would be required to actually cause a
> NULL pointer dereference here. This might be fixed cleaner by changing
>
>    211          } else {
>
> to
>
>    211          } else if (device_o) {
>
> The remainder of the warnings might be 'fixed' to shut-up cppcheck, but
> don't actually change anything.
>
For v2 of this patch, I added some stuff that reduces total compiled
size (changed functions to void since return value is not used).

If needed, I can split it up into two to deal with only cppcheck in
the first, and size optimizations in the second.

PS: No I have no idea why size gets reduced like this. The most
ridiculous example from a different project is where I changed a
function's return type from int to bool. GCC output went from 61440 to
57560 while clang was unaffected.

>
>
> _______________________________________________
> Lede-dev mailing list
> Lede-dev at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev