cppcheck static analysis

[setharnold]

Hello, I inspected OpenJPEG for the Ubuntu Linux distribution; as part of my review I ran the cppcheck static analysis tool on the 1.5.2 and 2.1.0 sources. I only inspected the 1.5.2 results (what is currently packaged in Ubuntu) and found most of the results looked legitimate with only a few false positives.

I did not inspect the 2.1.0 results beyond noticing that many looked similar to valid issues from 1.5.2.

I strongly recommend fixing all these issues -- many represent real bugs in the program that may be difficult to discover via other methods. It is worth fixing all the reported issues -- even the ones that represent FILE leaks when main() prematurely terminates -- as it makes it far easier to prevent new issues from entering the codebase when the output is usually clean.

Here's the list of issues it found:

[src/bin/jp2/opj_compress.c:1599]: (error) Memory leak: dirptr
[src/bin/jp2/opj_compress.c:1060]: (error) Memory leak: lMatrix
[src/bin/jp2/opj_compress.c:1599]: (error) Memory leak: parameters.cp_comment
[src/bin/jp2/opj_decompress.c:720]: (error) Memory leak: dirptr
[src/bin/jp2/opj_compress.c:631]: (error) Uninitialized variable: raw_signed
[src/bin/jp2/convert.c:130]: (error) Memory leak: tga
[src/bin/jp2/convert.c:1324]: (error) Resource leak: f
[src/bin/jp2/opj_dump.c:453]: (error) Memory leak: dirptr
[src/bin/jp2/convert.c:1880]: (error) Resource leak: fp
[src/bin/jp2/convert.c:3061]: (error) Resource leak: f
[src/bin/jp2/opj_dump.c:439]: (error) Memory leak: l_data
[src/bin/jp2/opj_dump.c:453]: (error) Memory leak: l_data
[src/bin/jp2/opj_dump.c:461]: (error) Memory leak: l_data
[src/bin/jp2/opj_dump.c:466]: (error) Memory leak: l_data
[src/bin/jp2/opj_dump.c:477]: (error) Memory leak: l_data
[src/bin/jp3d/convert.c:300]: (error) Resource leak: f
[src/bin/jp3d/convert.c:690]: (error) Resource leak: fdest
[src/bin/jpwl/convert.c:130]: (error) Memory leak: tga
[src/bin/jpwl/convert.c:1302]: (error) Resource leak: f
[src/bin/jpwl/convert.c:434]: (error) Resource leak: fdest
[src/bin/jpwl/convert.c:714]: (error) Resource leak: IN
[src/bin/jpwl/convert.c:1462]: (error) Resource leak: fdest
[src/bin/jpwl/convert.c:1437]: (error) Memory leak: name
[src/bin/jpwl/convert.c:2889]: (error) Resource leak: f
[src/bin/jpwl/opj_jpwl_compress.c:420]: (error) Memory leak: fname
[src/bin/jpwl/opj_jpwl_compress.c:1562]: (error) Memory leak: dirptr
[src/bin/jpwl/opj_jpwl_decompress.c:564]: (error) Memory leak: dirptr
[src/bin/jpwl/opj_jpwl_compress.c:1728]: (error) Resource leak: f
[src/bin/jpwl/opj_jpwl_compress.c:1789]: (error) Resource leak: f
[src/bin/jpwl/opj_jpwl_compress.c:1562]: (error) Memory leak: parameters.cp_comment
[src/bin/mj2/meta_out.c:684]: (error) fprintf format string requires 1 parameter but only 0 are given.
[src/bin/mj2/meta_out.c:702]: (error) fprintf format string requires 1 parameter but only 0 are given.
[src/bin/mj2/mj2_to_metadata.c:269]: (error) Resource leak: file
[src/bin/mj2/meta_out.c:1836]: (error) Invalid number of character '(' when these macros are defined: 'NOTYET'.
[src/bin/mj2/opj_mj2_compress.c:691]: (error) Resource leak: mj2file
[src/bin/mj2/opj_mj2_decompress.c:101]: (error) Resource leak: outfile
[src/bin/mj2/opj_mj2_decompress.c:161]: (error) Common realloc mistake: 'frame_codestream' nulled but not freed upon failure
[src/bin/mj2/opj_mj2_extract.c:140]: (error) Memory leak: frame_codestream
[src/bin/wx/OPJViewer/source/OPJThreads.cpp:892]: (error) Mismatching allocation and deallocation: buffer
[src/bin/wx/OPJViewer/source/wxj2kparser.cpp:188]: (error) Array 'marker_val[19]' accessed at index 23, which is out of bounds.
[src/bin/wx/OPJViewer/source/wxj2kparser.cpp:188]: (error) Array 'marker_val[20]' accessed at index 23, which is out of bounds.
[src/bin/wx/OPJViewer/source/wxj2kparser.cpp:188]: (error) Array 'marker_val[23]' accessed at index 23, which is out of bounds.
[src/lib/openjp2/j2k.c:8264]: (error) Memory leak: cstr_index
[src/lib/openjp2/j2k.c:3762]: (error) Invalid number of character '{' when these macros are defined: 'CLEAN_MSD'.
[src/lib/openjp3d/jp3d.c:1018]: (error) Uninitialized variable: numbands
[src/lib/openjp3d/tcd.c:1560]: (error) Uninitialized variable: l
[src/lib/openjp3d/tcd.c:1570]: (error) Uninitialized variable: l
[src/lib/openjpip/j2kheader_manager.c:120]: (error) Uninitialized variable: COD
[src/lib/openmj2/j2k.c:1548]: (error) Common realloc mistake: 'data' nulled but not freed upon failure
[src/lib/openmj2/mj2.c:1456]: (error) Array 'tk.urn[urn_num].name[2]' accessed at index 2, which is out of bounds.
[src/lib/openmj2/mj2.c:1457]: (error) Array 'tk.urn[urn_num].name[2]' accessed at index 3, which is out of bounds.
[src/lib/openmj2/mj2.c:1494]: (error) Array 'tk.urn[urn_num].name[2]' accessed at index 2, which is out of bounds.
[src/lib/openmj2/mj2.c:1495]: (error) Array 'tk.urn[urn_num].name[2]' accessed at index 3, which is out of bounds.
[src/lib/openmj2/mj2.c:2709]: (error) Common realloc mistake: 'src' nulled but not freed upon failure
[src/lib/openmj2/mj2.c:2713]: (error) Memory leak: src
[src/lib/openmj2/mj2_convert.c:63]: (error) Resource leak: f
[tests/test_tile_encoder.c:239]: (error) Memory leak: l_data
[thirdparty/libtiff/tif_luv.c:587]: (error) Shifting 32-bit value by 32 bits is undefined behaviour
[thirdparty/libz/gzread.c:345]: (error) Uninitialized variable: n
[thirdparty/libz/gzwrite.c:137]: (error) Uninitialized variable: n
[thirdparty/libz/inflate.c:1475]: (error) Shifting a negative value is undefined behaviour

Thanks

[julienmalik]

On current master (5c5ae1d) I get :

$ cppcheck --force $(find . -name *.c) > /dev/null
[src/bin/jp2/opj_dump.c:451]: (error) Memory leak: l_data
[src/bin/jp3d/convert.c:300]: (error) Resource leak: f
[src/bin/jp3d/convert.c:690]: (error) Resource leak: fdest
[src/bin/jpip/opj_jpip_addxml.c:91]: (error) Resource leak: fp
[src/bin/jpwl/convert.c:130]: (error) Memory leak: tga
[src/bin/jpwl/convert.c:434]: (error) Resource leak: fdest
[src/bin/jpwl/convert.c:714]: (error) Resource leak: IN
[src/bin/jpwl/convert.c:1302]: (error) Resource leak: f
[src/bin/jpwl/convert.c:1462]: (error) Resource leak: fdest
[src/bin/jpwl/convert.c:1437]: (error) Memory leak: name
[src/bin/jpwl/convert.c:3211]: (error) Uninitialized variable: height
[src/bin/jpwl/opj_jpwl_compress.c:369]: (error) Resource leak: dir
[src/bin/jpwl/opj_jpwl_compress.c:394]: (error) Resource leak: dir
[src/bin/jpwl/opj_jpwl_compress.c:420]: (error) Memory leak: fname
[src/bin/jpwl/opj_jpwl_compress.c:1728]: (error) Resource leak: f
[src/bin/jpwl/opj_jpwl_compress.c:1789]: (error) Resource leak: f
[src/bin/jpwl/opj_jpwl_decompress.c:171]: (error) Resource leak: dir
[src/bin/jpwl/opj_jpwl_decompress.c:196]: (error) Resource leak: dir
[src/bin/mj2/meta_out.c:684]: (error) fprintf format string has 1 parameters but only 0 are given.
[src/bin/mj2/meta_out.c:702]: (error) fprintf format string has 1 parameters but only 0 are given.
[src/bin/mj2/meta_out.c:1836]: (error) Invalid number of character (() when these macros are defined: 'NOTYET'.
[src/bin/mj2/mj2_to_metadata.c:269]: (error) Resource leak: file
[src/bin/mj2/opj_mj2_compress.c:691]: (error) Resource leak: mj2file
[src/bin/mj2/opj_mj2_decompress.c:161]: (error) Common realloc mistake: 'frame_codestream' nulled but not freed upon failure
[src/bin/mj2/opj_mj2_decompress.c:101]: (error) Resource leak: outfile
[src/bin/mj2/opj_mj2_extract.c:140]: (error) Memory leak: frame_codestream
[src/lib/openjp2/t1_generate_luts.c:63]: (error) Uninitialized variable: t
[src/lib/openjp3d/t1.c:739]: (error) Uninitialized variable: t
[src/lib/openjpip/j2kheader_manager.c:120]: (error) Uninitialized variable: COD
[src/lib/openmj2/j2k.c:1548]: (error) Common realloc mistake: 'data' nulled but not freed upon failure
[src/lib/openmj2/mj2.c:2709]: (error) Common realloc mistake: 'src' nulled but not freed upon failure
[src/lib/openmj2/mj2.c:2713]: (error) Memory leak: src
[src/lib/openmj2/mj2_convert.c:63]: (error) Resource leak: f
[tests/test_tile_encoder.c:239]: (error) Memory leak: l_data
[wrapping/java/openjp2/JavaOpenJPEGDecoder.c:158]: (error) Resource leak: dir
[wrapping/java/openjp2/JavaOpenJPEGDecoder.c:183]: (error) Resource leak: dir

[julienmalik]

So the main component (lib/openjp2 and bin/jp2) are almost cppcheck-free.
I just submitted #734 for opj_dump.

I did not understand the t1_generate_luts.c:63 warning, but initializing t at the beginning of the function probably won't harm.

[szukw000]

@detonin, @julienmalik,

using cppcheck-1.73, I have made a cppckeck on the whole openjpeg-master-2016-04-13 library.

Without directories:
====================
src/bin/wx because it is broken
src/thirdparty
src/wrapping because it is broken

Skipped:
========
The scope of the variable 'xxx' can be reduced.
Variable 'xxx' is assigned a value that is never used.
(warning) scanf without field width limits can crash with huge input data.
Unused variable: xxx
(portability) Casting between integer* and float* which have an incompatible binary data representation.

Unsolved:
=========
[src/lib/openmj2/t1.c:1124]: (warning) Redundant code: Found a statement that begins with numeric constant.
[src/lib/openmj2/t1.c:1137]: (warning) Redundant code: Found a statement that begins with numeric constant.

Problems:
=========
src/lib/openjp3d: possible failures remain
src/lib/openjpip: possible failures remain

Most malloc/calloc/realloc tests have been done. The resulting DIF file is 176'715 Bytes large.

The worst library is the JP3D library. Followed by the JPIP library.

Does anybody work on cppckeck tests? Then I would give up.

winfried

[julienmalik]

@szukw000 , not sure to understand : you have a patch ready for fixing some cppcheck warnings ?

[szukw000]

@julienmalik : yes, I have a patch. But it is not complete. And before I spend more time I would
like to know: does another one work on fixing cppcheck errors and warnings? The fix is very time
consuming.

winfried

[szukw000]

@julienmalik, the attached file shows some problems.

cidx_manager.c, ppix_manager.c, thix_manager.c: they return a length of 0
in opj_write_tpix(), opj_write_thix(), opj_write_cidx() (unused).

E.g. in line 88, cidx_manager.c:

box[num_box].length = (OPJ_UINT32)opj_write_tpix( offset, cstr_info, j2klen, cio,p_manager);

In opj_write_manf() this length is written:

opj_write_bytes( l_data_header, box[i].length, 4);

What is the effect if a length of 0 is met anywhere?

In j2k.c the condition

if (!JPWL_ASSUME || JPWL_ASSUME)

is reduced to
if (!JPWL_ASSUME)

because of '!JPWL_ASSUME == JPWL_ASSUME'.

What is the result of the correction?

opj_event_msg(p_manager, JPWL_ASSUME ? EVT_WARNING : EVT_ERROR,

JPWL_ASSUME means EVT_WARNING, !JPWL_ASSUME means EVT_ERROR.

if (!JPWL_ASSUME) {
  opj_event_msg(p_manager, EVT_ERROR, "JPWL: giving up\n");
}

In jp2.c, line 646 ff, jp2 first is used, then checked whether it is NULL.
This bug is found at several places.

The cielab test at line 1408:

+           if(cielab == NULL){
+               opj_event_msg(p_manager, EVT_ERROR, "Not enough memory for cielab\n");
+               return OPJ_FALSE;
+           }

has been reported as a fix elsewhere. The fix has never been applied.
This is boring. And time wasting.

winfried
openjp2-dif.txt

[szukw000]

@detonin, @julienmalik .

Here is the full patch.

winfried
Cppcheck-dif.txt

[szukw000]

What the UBUNTU people have criticised is true. The OPENJPEG library
is unsafe. My patch does make the library a little bit more safe.

Some comments.

src/lib/openmj2/j2k.c:

if (!JPWL_ASSUME || JPWL_ASSUME)

does mean the same. So with

opj_event_msg(j2k->cinfo, JPWL_ASSUME ? EVT_WARNING : EVT_ERROR,

only

if (!JPWL_ASSUME)
opj_event_msg(j2k->cinfo, EVT_ERROR, "JPWL: giving up\n");

is used.

src/lib/openjpip:

To remove in src/lib/openjpip

undefined reference to `opj_malloc'

I added in CMakeLists.txt '${OPENJPEG_LIBRARY_NAME}'

if(BUILD_JPIP_SERVER)
add_library(openjpip_server STATIC ${OPENJPIP_SRCS} ${SERVER_SRCS})
target_link_libraries(openjpip_server ${FCGI_LIBRARIES} ${CURL_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT} ${OPENJPEG_LIBRARY_NAME})

and in some H.files

#include "opj_includes.h"

At several places after 'return' the comment

/* FIXME szukw000 */

has been added. The FIXME shall remember that it remains unclear what
should be done. Several functions are 'void' instead of returning
'FALSE' or 'TRUE' or 'NULL' which can be tested as in openjp2.

With 'return;' only it may happen that a 'delete_FUNC()' tries to
free a NULL component. Which may or may not be allowed.

Some bugs are very, very old. Patches have been ignored. E.g.

src/lib/openmj2/mj2.h

 typedef struct mj2_urn {
-  int name[2];
+  int name[4];
   int location[4];
 } mj2_urn_t;

This ignorance does not make the library more safe.

The 'src/bin/fltk/flviewer' has been added as Antonin wanted a
working viewer for version 2.1.x; here it is.

winfried

linux-cmake-build-flviewer.sh:

#!/bin/sh
cmake -DBUILD_DOC:bool=off -DBUILD_PKGCONFIG_FILES:bool=on -DCMAKE_BUILD_TYPE:string="Release" -DBUILD_FLVIEWER:bool=on -DBUILD_FLVIEWER_WITH_SHARED_LIBS:bool=on -DBUILD_JAVA:bool=off -DBUILD_JPIP_SERVER:bool=on -DBUILD_SHARED_LIBS:bool=on -DBUILD_MJ2:bool=on -DBUILD_JP3D:bool=on -DBUILD_JPWL:bool=on -DBUILD_JPIP:bool=on -DCMAKE_INSTALL_PREFIX=/usr/local/opj2 -DFLTK_SKIP_OPENGL:bool=on ..

win32-cmake-build-flviewer.bat:

cmake -G "NMake Makefiles" -DBUILD_DOC:bool=off -DBUILD_PKGCONFIG_FILES:bool=off -DCMAKE_BUILD_TYPE:string="Release" -DBUILD_FLVIEWER:bool=on -DBUILD_FLVIEWER_WITH_SHARED_LIBS:bool=on -DBUILD_JAVA:bool=off -DBUILD_JPIP_SERVER:bool=on -DBUILD_SHARED_LIBS:bool=on -DBUILD_MJ2:bool=on -DBUILD_JP3D:bool=on -DBUILD_JPWL:bool=on -DBUILD_JPIP:bool=on -DCMAKE_INSTALL_PREFIX:path="C:/Users/Public" -DFLTK_SKIP_OPENGL:bool=on -DBUILD_WITH_GERMAN:bool=off ..

[julienmalik]

@szukw000 It would really help if you looked into how to submit pull requests instead of raw patches.
In the mean time, thanks for the work so far, and here is your patch split in a few pull requests :
#739
#740
#741
#742
#743
#744

[maddin200]

Maybe it is more convenient to propose a cppcheck message as a single issue because it is easier to fix one file and test. As well there is some kind of four eyes principle and more people can supply (of course smaller) patches.
Bye Martin

[setharnold]

I just re-checked openjpeg-2.1.1 with cppcheck 1.72 and the results are significantly better:

$ cppcheck -q -j30 .
[bin/mj2/opj_mj2_extract.c:140]: (error) Memory leak: frame_codestream
[bin/mj2/opj_mj2_decompress.c:101]: (error) Resource leak: outfile
[bin/mj2/opj_mj2_decompress.c:161]: (error) Common realloc mistake: 'frame_codestream' nulled but not freed upon failure
[bin/mj2/mj2_to_metadata.c:269]: (error) Resource leak: file
[bin/mj2/opj_mj2_compress.c:691]: (error) Resource leak: mj2file
[bin/jpwl/opj_jpwl_decompress.c:564]: (error) Memory leak: dirptr
[bin/jp3d/convert.c:300]: (error) Resource leak: f
[bin/jp2/opj_compress.c:1660]: (error) Memory leak: dirptr
[bin/jp3d/convert.c:690]: (error) Resource leak: fdest
[bin/jpwl/opj_jpwl_compress.c:420]: (error) Memory leak: fname
[bin/jpwl/opj_jpwl_compress.c:1562]: (error) Memory leak: dirptr
[bin/mj2/meta_out.c:684]: (error) fprintf format string requires 1 parameter but only 0 are given.
[bin/mj2/meta_out.c:702]: (error) fprintf format string requires 1 parameter but only 0 are given.
[bin/wx/OPJViewer/source/wxj2kparser.cpp:188]: (error) Array 'marker_val[19]' accessed at index 23, which is out of bounds.
[bin/jpwl/opj_jpwl_compress.c:1728]: (error) Resource leak: f
[bin/jpwl/opj_jpwl_compress.c:1789]: (error) Resource leak: f
[bin/jpwl/opj_jpwl_compress.c:1562]: (error) Memory leak: parameters.cp_comment
[bin/jpwl/convert.c:130]: (error) Memory leak: tga
[bin/jpwl/convert.c:1302]: (error) Resource leak: f
[bin/jpwl/convert.c:434]: (error) Resource leak: fdest
[bin/jpwl/convert.c:714]: (error) Resource leak: IN
[bin/jpwl/convert.c:1462]: (error) Resource leak: fdest
[bin/jpwl/convert.c:1437]: (error) Memory leak: name
[bin/wx/OPJViewer/source/wxj2kparser.cpp:188]: (error) Array 'marker_val[20]' accessed at index 23, which is out of bounds.
[bin/wx/OPJViewer/source/wxj2kparser.cpp:188]: (error) Array 'marker_val[23]' accessed at index 23, which is out of bounds.
[lib/openjpip/j2kheader_manager.c:120]: (error) Uninitialized variable: COD
[bin/mj2/meta_out.c:1836]: (error) Invalid number of character '(' when these macros are defined: 'NOTYET'.
[lib/openmj2/mj2_convert.c:63]: (error) Resource leak: f
[lib/openmj2/j2k.c:1548]: (error) Common realloc mistake: 'data' nulled but not freed upon failure
[lib/openmj2/mj2.c:1456]: (error) Array 'tk.urn[urn_num].name[2]' accessed at index 2, which is out of bounds.
[lib/openmj2/mj2.c:1457]: (error) Array 'tk.urn[urn_num].name[2]' accessed at index 3, which is out of bounds.
[lib/openmj2/mj2.c:1494]: (error) Array 'tk.urn[urn_num].name[2]' accessed at index 2, which is out of bounds.
[lib/openmj2/mj2.c:1495]: (error) Array 'tk.urn[urn_num].name[2]' accessed at index 3, which is out of bounds.
[lib/openmj2/mj2.c:2709]: (error) Common realloc mistake: 'src' nulled but not freed upon failure
[lib/openmj2/mj2.c:2713]: (error) Memory leak: src

Thanks for fixing the other issues; it'd sure be nice to bring this list down to zero.

Thanks