Bogus 'may be used uninitialized

Question

I am getting an unitialized warning in the following code, and I am stumped trying to figure out why. I can't see a code path where it is used uninitialized.Can anyone please help? Also, I could use some adivce on if my gotos are not used well or if there is a cleaner way of doing this.

 In function ‘handle_comp_enc’:
fs/compress.c:101:8: warning: ‘write_cdata’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   kfree(write_cdata);

Code:

#define ENC  (1UL << 1)
#define ZIP  (1UL << 2)
#define ENC_ZIP_ENABLED(cmp_enc_flags) ((cmp_enc_flags) & (ENC | ZIP)) == (ENC | ZIP)


int handle_comp_enc(unsigned long comp_enc_flags, unsigned char *read_data,
                        size_t read_len, unsigned char *write_data, size_t *write_len2) {
        unsigned char *write_cdata, *rd_enc_data;
        size_t write_clen, enc_src_len;
        int err;
        if (ENC_ZIP_ENABLED(comp_enc_flags)){
                write_cdata = kmalloc(get_compress_fsize(PAGE_SIZE), GFP_KERNEL);
                if (!write_cdata) {
                        err  = -ENOMEM;
                        goto zip_only;
                }
        }
        else if(!(comp_enc_flags & ENC))
                write_cdata = write_data;
        else{
                rd_enc_data = read_data;
                enc_src_len = read_len;
                goto enc_only;
        }
        err = do_compress(read_data, read_len, write_cdata, &write_clen);
        if (err < 0) {
                goto out_enc_zip;
        }
        if (!(comp_enc_flags & ENC)) {
                *write_len2 = write_clen;
                goto zip_only;
        }
        rd_enc_data = write_cdata;
        enc_src_len = write_clen;
enc_only:
        err = do_skcipher_encrypt(rd_enc_data, enc_src_len, write_data, write_len2);
        if (err < 0) {
        }
out_enc_zip:
        if (ENC_ZIP_ENABLED(comp_enc_flags))
                kfree(write_cdata);
zip_only:
        return err;
}

Stargateur · Accepted Answer · 2018-04-25T10:28:04.017

3

Compiler try it's best to produce warning, as the message say "maybe", the compiler don't know that ENC_ZIP_ENABLED(comp_enc_flags) will be false at the label out_enc_zip. Your code don't use an uninitialized value.

That say, I strongly disagree about your use case of goto, your code is unreadable, I take a lot of time just to understand where the code was going.

Your code could be simplified a lot, I'm not sure at 100% that this code have the same behavior as I said your code is hard to read:

#define ENC  (1UL << 1)
#define ZIP  (1UL << 2)

int handle_comp_enc(unsigned long comp_enc_flags, unsigned char *read_data,
                    size_t read_len, unsigned char *write_data, size_t *write_len2) {
        if ((comp_enc_flags & (ENC | ZIP)) == (ENC | ZIP)) {
            unsigned char *tmp = kmalloc(get_compress_fsize(PAGE_SIZE), GFP_KERNEL);
            if (!tmp) {
                    return -ENOMEM;
            }
            size_t size;
            int err = do_compress(read_data, read_len, tmp, &size);
            if (!(err < 0)) {
                err = do_skcipher_encrypt(tmp, size, write_data, write_len2);
            }
            kfree(tmp);
            return err;
        }
        else if (!(comp_enc_flags & ENC)) {
              return do_compress(read_data, read_len, write_data, write_len2);
        }
        else {
            return do_skcipher_encrypt(read_data, read_len, write_data, write_len2);
        }
}

edited Apr 25 '18 at 10:28

answered Apr 25 '18 at 09:37

Stargateur

24,473
8
65
91

I agree this is true in this particular code since the error handling is rather simple, i.e. you can simply return. If the error handling was such that you had to do a couple of tasks before returning from each error, like freeing something, closing an fd etc., you would have to repeat that at each error. In that case, goto would have been much more readable. Also, the purpose of the function is(since you said that it is hard to read): either use both compression and encryption or just one of them as dictated by the comp_enc flag. – abjoshi - Reinstate Monica Apr 25 '18 at 09:53
@abjoshi I'm not here for talking about good use case of goto, I perfectly aware of it, if you need an exemple, look [here](https://stackoverflow.com/a/245761/7076153). But I talking of your code. And I think your code is a typical situation where goto is badly wrong, you ask me why, I'm not good for talking, I prefer show code. And I show you it's perfectly possible without repeat code to not use goto in your case. I think I have nothing more to add that "your gotos was useless". – Stargateur Apr 25 '18 at 10:01
Provided that Stargateur's code is functionally equivalent, it wins over the original code since it is *much* easier to follow. It clearly shows the great advantage of structured (goto-less) programming. However, the code is still not *purely structured*. – August Karlstrom Apr 25 '18 at 10:20
Though this is not functionally equivalent, I concede I could have done better. – abjoshi - Reinstate Monica Apr 25 '18 at 10:34

score 1 · Answer 2 · answered Apr 25 '18 at 10:02

Yeah it looks like a false positive. In your if-else if-else, you only initialize the variable inside the if and else if statements. Apparently you got the tool confused with goto or some such.

But that's not really important, as the origin of the problems is the function design. You don't default initialize variables and you have a tight coupling between memory allocation and the actual algorithm. The use of goto here is ok but it reduces readability somewhat.

I would split this in two functions, where you leave memory handling and error handling to an outer function. Something along the lines of this pseudo code would be much more readable:

int wrapper_function ( ... )
{
  unsigned char *write_cdata = NULL;
  int err = initialize_me_to_something.

  if(ENC_ZIP_ENABLED(comp_enc_flags))
  {
    write_cdata = kmalloc (...
    if(write_cdata == NULL)
    {
      return -ENOMEM;
    }
  }
  else
  {
    if(!(comp_enc_flags & ENC)
    {
      write_cdata = write_data;
      ...
    }
    else
    { // some special case
      err = do_skcipher_encrypt(...
      return err;
    }
  }


  err = do_the_actual_job(write_cdata, otherparameters); 

  if (err < 0) 
  {
    cleanup();
  }

  return err;
}

goto is not necessarily evil, but neither are multiple return statements. They are both frowned upon with more or less rational arguments provided. However, multiple return statements tend to improve readability quite a bit over the "on error goto" pattern. Most importantly, they tend to naturally give a better program design with multiple small functions instead of a single big one.

As a side effect, you get rid of some extra branching instructions, which might give a slight performance improvement.

Though this is not functionally equivalent, I concede I could have done better in *this* case. Like I said in response to Stargaetur, had the error handling been more complex, this would have been a harder problem. Regarding your point about getting rid of branching instructions: that may be true. But consider the scenario, where you have to call your outer cleanup functions(as you mentioned above) many times. If they are small(and they usually are), compiler may inline them and they may end up being part of your code. Second, this would also increase your instruction cache footprint. — abjoshi - Reinstate Monica, Apr 25 '18 at 10:39
@abjoshi Readability is the most important here though, not some mini-optimizations. Had the error handling been more complex, this would scale much better. An internal function can then just `return` upon error, instead of creating something complex with nested statements or multiple goto labels. — Lundin, Apr 25 '18 at 10:48

Bogus 'may be used uninitialized

2 Answers2