Uninitialized variable in EMF/WMF input

Bug #1248753 reported by David Mathog on 2013-11-06
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Inkscape
Undecided
David Mathog

Bug Description

In the developer's mailing list Johan Engelen said that clang was giving a warning in wmf-input.cpp. I don't have clang,
but the section of code suggested that the warning was for variable dibparams. g++ on a normal build was not emitting this warning, but I will try to resolve this in any case. (There should have been other warnings, since similar code structures are used in about 5 places.)

The attached patch should eliminate the clang warnings. Other than that, the program should work exactly the same before and after the patch - unless you encounter a file containing an image which is NOT U_DIB_RGB_COLORS. I have never seen a file with this field set to another value, but apparently there is a bitmap type which is doubly indirect, color value = bitmap -> color table -> palette table. That mode is not supported by libUEMF. libUEMF does support color value = bitmap, and color value = bitmap -> Color table, both of which have the U_DIB_RGB_COLORS DIBColors enumeration value set in the UsageSrc field of the relevant record types. Anyway, this not seen before in the wild case would be:

IF an EMF file contains records with UsageSrc != U_DIB_RGB_COLORS
AND the uninitialized dibparams value happened to be U_BI_JPEG or U_BI_PNG
THEN that image would display differently before and after the patch

but I cannot provide a test example, as I have no files that are not U_DIB_RGB_COLORS.

David Mathog (mathog) wrote :
Johan Engelen (johanengelen) wrote :

Thanks for the patch.
The goal is not to suppress warnings. The goal is to write good code. Just initialize every variable upon defining it, and commit that to trunk. Thanks.

Johan Engelen (johanengelen) wrote :
Download full text (10.7 KiB)

Here the actual warning list:

../../src/extension/internal/wmf-inout.cpp:606:8: warning: variable 'width' is
      used uninitialized whenever '||' condition is true
      [-Wsometimes-uninitialized]
    if((iUsage != U_DIB_RGB_COLORS) ||
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../src/extension/internal/wmf-inout.cpp:624:13: note: uninitialized use
      occurs here
            width, // Width of pixel array in record
            ^~~~~
../../src/extension/internal/wmf-inout.cpp:606:8: note: remove the '||' if its
      condition is always false
    if((iUsage != U_DIB_RGB_COLORS) ||
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../src/extension/internal/wmf-inout.cpp:605:19: note: initialize the variable
      'width' to silence this warning
    int32_t width, height, colortype, numCt, invert;
                  ^
                   = 0
../../src/extension/internal/wmf-inout.cpp:606:8: warning: variable 'height' is
      used uninitialized whenever '||' condition is true
      [-Wsometimes-uninitialized]
    if((iUsage != U_DIB_RGB_COLORS) ||
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../src/extension/internal/wmf-inout.cpp:625:13: note: uninitialized use
      occurs here
            height, // Height of pixel array in record
            ^~~~~~
../../src/extension/internal/wmf-inout.cpp:606:8: note: remove the '||' if its
      condition is always false
    if((iUsage != U_DIB_RGB_COLORS) ||
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../src/extension/internal/wmf-inout.cpp:605:27: note: initialize the variable
      'height' to silence this warning
    int32_t width, height, colortype, numCt, invert;
                          ^
                           = 0
../../src/extension/internal/wmf-inout.cpp:606:8: warning: variable 'dibparams'
      is used uninitialized whenever '||' condition is true
      [-Wsometimes-uninitialized]
    if((iUsage != U_DIB_RGB_COLORS) ||
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../src/extension/internal/wmf-inout.cpp:641:8: note: uninitialized use occurs
      here
    if(dibparams == U_BI_JPEG || dibparams==U_BI_PNG){
       ^~~~~~~~~
../../src/extension/internal/wmf-inout.cpp:606:8: note: remove the '||' if its
      condition is always false
    if((iUsage != U_DIB_RGB_COLORS) ||
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../src/extension/internal/wmf-inout.cpp:597:19: note: initialize the variable
      'dibparams' to silence this warning
    int dibparams;
                  ^
                   = 0
../../src/extension/internal/wmf-inout.cpp:606:8: warning: variable 'numCt' is
      used uninitialized whenever '||' condition is true
      [-Wsometimes-uninitialized]
    if((iUsage != U_DIB_RGB_COLORS) ||
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../src/extension/internal/wmf-inout.cpp:622:13: note: uninitialized use
      occurs here
            numCt, // DIB color table number of entries
            ^~~~~
../../src/extension/internal/wmf-inout.cpp:606:8: note: remove the '||' if its
      condition is always false
    if((iUsage != U_DIB_RGB_COLORS) ||
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../src/extension/internal/wmf-inout.cpp:605:45: note: initialize the variable
      'numCt' to silence this warnin...

David Mathog (mathog) wrote :

clang seems to be making assumptions which are not true for this code and based on that emitting the warnings you see.

Notice that px, rgba_px, and mempng.buffer are all initialized to NULL. That is important because those are used in various places as flags to indicate whether or not processing can continue on down a given path.

If the iUsage test is true, which only happens if a known but unsupported DIB type is found, then DIB_to_RGBA is called.
It checks to see if px is NULL. Which it is, and so it returns. rgba_px is unchanged and toPNG is never called.
A little further down it sees that mempng.buffer is still NULL, so a random 3 x 4 blotch is inserted where the uninterpretable image should have been.

clang does not know what happens px, rgba_px, and mempng.buffer inside the functions, yet it seems to be assuming somehow that it does know. For instance, it seems to be ignoring the "&& rgb_px" test, so it thinks all of those variables are going to be passed in uninitialized in this case, and issues all of those warnings.

If the iUsage test is false, which is the normal case, then wget_dib_params is called, and that conditionally fills in the &variable values.

The variables clang warns about cannot be initialized at the top of the method, as they have no default values. (What are the default width and height of an unknown bitmap, including one which is invalid?)

If dibparams is 0 then one of Microsoft's RGB bitmap formats was found in the DIB, and DIB_to_RGBA, then toPNG are called to load that bitmap into mempng.buffer. In this case all of those &variables are actually used - but when they are, they have values assigned to them.

If dibparams indicates a PNG or JPG, then the binary data is already in the px buffer, so no need to call DIB_to_RGBA or toPNG, and all of the other variables are irrelevant, and remain undefined and unused.

Is clang giving similar warnings for emf-inout.cpp? If it is consistent it should be, since nearly the identical code is used there.

LucaDC (dicappello) wrote :

@David,
your argumentations are technically correct, but I think you are missing a point I often see people miss.
Having a code being correct just as a consequence of such a deep analysis of the program flow is somewhat dangerous if not misleading. This means that if someone that does not have a clear picture of what's happening makes some modifications that change the program flow, maybe even only slightly, some bad thing could happen.
Personally I tend to judge code correctness based on the assumption that the program flow may take different paths even in cases where it's evident to a human reader that it's not possible. I just try to read the code from a dummy programmer point of view.
If a variable has a long and lifetime and an articulated processing (I mean, with at least two independent conditional paths), better initialize it to a neutral value at the beginning, just to be sure. In my opinion, not initializing in such cases is at least an half bug (to be nice).

David Mathog (mathog) wrote :

I always try to initializ variables when doing so makes sense. Notice that the pointers to various types of memory blobs px, rgba_px, and mempng.buffer are all initialized, because during processing these memory blobs either appear or they don't.
I added an initialization for dibparams because there was a remote possibility that it might actually be used uninitialized. The variables that clang is incorrectly (as far as I can tell) complaining about are not initialized because there is no valid default value to set them to. Sure, they could all be set to zero, or -1, but is the code more correct if it sets values for image width and length that are nonphysical?

I would be more concerned if I were the clang users about finding out why clang is giving these warnings. At most it should say that these variables MIGHT be used uninitialized, which is as much as it should be able to determine since it cannot know what goes on inside the called functions. Instead it says they ARE being used uninitialized, and that is just wrong if the code compiles properly I have been around long enough to have seen several instances where compilers did not actually generate valid instructions. It is unlikely, but possible, that this code is uncovering a clang compiler error such that clang generated code really would use those variables uninitialized, for instance, by not calling wget_dib_params before calling calling DIB_to_RGBA. If clang is working consistently, these warnings should be showing up for emf-inout.cpp too, as there are nearly identical functions there, which Johan must tell me as I don't use clang.

Johan, what happens when clang compiles this fragment?

int test1(int *var); // prototype
int test2(int var); // prototype

int var;
if(test1(&var)){
   std::cout << "Result: " << test2(var) << std::endl;
}

Johan Engelen (johanengelen) wrote :

David, you are writing very buggy code. I will ask again, please initialize ALL variables.
I don't care if you think you are smarter than all of us. I do care about Inkscape's codebase, and I want it to be understandable, maintainable, and bug-free upon inspection.

You are the first person that makes me think about how we deal with revoking someone's trunk commit access.

David Mathog (mathog) wrote :

A little bit of hyperbole is fine, but "very buggy" is hardly an appropriate categorization just because clang on your machine throws warnings. Please keep in mind that those files compile with zero warnings with g++ with very stringent checking turned on. Also the emf and wmf functions were tested extensively with valgrind and EMF/WMF test files while reading and writing the most complex metafiles I could find or generate myself, and that turned up no memory leaks in their code and no uninitialized memory accesses. (That did find leaks attributable to other Inkscape methods which must be called and leak, see bug 1043571.)

So I installed clang on my machine and compiled wmf-inout.cpp (just replaced "g++" with "clang"). This is all that it emitted:

  clang: warning: argument unused during compilation: '-fopenmp'

Not finding any of the warnings you cite, I then tried each of -O0, -O1, -O2, and -O3, and that still did not generate the warnings.

This is on Ubuntu 12.04.3 LTS, clang 3.0-6ubuntu3. 32 bit.

Initializing all of those variables will silence clang on your platform, but before doing that I want to see if there is really a bug in this somewhere, because if there is initializing those variables is going to mask it.

What platform are you using? Maybe there is a 32 bit vs. 64 bit issue?

ScislaC (scislac) wrote :

David: He is using Windows (I do not know version or if it's 32 or 64 bit).

David Mathog (mathog) wrote :

Unclear if this will resolve the clang issue, as I have not been able to replicate it. However, the attached patch does clean up some of the code in [ew]mf-inout.cpp, with the intent of making it easier to read. It also adds metafile-inout.cpp and metafile-inout.h as the parent class of [ew]mf-inout. This brings it into line with the equivalent *print functions, which were already arranged like that.

Tested and works on both linux and windows.

If it tests OK on OSX, please commit.

David Mathog (mathog) wrote :

Attached script can be used for generic testing purposes, when a patch is not supposed to change anything. It can be used to pick up even single bit changes in the large and complex test file.

To use it first apply patches;

changes_2013_10_18b.patch from bug 12411797 (uemf header changes)
changes_2013_11_12b.patch from bug 1250250 (emf/wmf offset)
btool.patch from bug 1251405 (btool build issue, if on Windows)

obtain and build libUEMF from sourceforge (for the reademf and readwmf programs, and the test_libuemf_ref.[ew]mf files)

Modify testew.sh to match your system, run it.

These should differ only in the name field: diff test1_emf.txt test3_emf.txt

These should not differ at all: diff test1_wmf.txt test3_wmf.txt

Keep a copy of test1_emf.txt and test1_wmf.txt somewhere safe before making any changes, then compare these old
ones with the output of testew.sh.

David Mathog (mathog) wrote :

Oops, that should have been bug #1241797, not 12411797.

su_v (suv-lp) on 2013-11-20
tags: added: build
David Mathog (mathog) wrote :

Will one of the OS X developers please apply and test these patches:

changes_2013_11_15a.patch (this bug)
changes_2013_11_12b.patch (bug #1250250)
changes_2013_10_18b.patch (bug #1241797)

and then commit them if they are OK on that platform?

They have all already been tested on Windows and Linux.

Thanks.

su_v (suv-lp) wrote :

@David - sorry for the delay wrt testing & committing the patch you provided back in November: quite a few other EMF/WMF-related patches have been committed in the meantime, could you give a brief update about the status of this report based on current trunk (r13008), and the patch currently attached (changes_2013_11_15a.patch)?

Changed in inkscape:
status: New → In Progress
assignee: nobody → David Mathog (mathog)
milestone: none → 0.91
su_v (suv-lp) on 2014-02-08
tags: added: code-design
David Mathog (mathog) wrote :

This looks like it is the last piece of the code cleanup that has not yet made it into trunk.

su_v (suv-lp) wrote :

Patch 'changes_2014_03_07a_wmfonly.patch' committed in revision revision 13167.

Changed in inkscape:
milestone: 0.91 → none
status: In Progress → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers