kyz/libmspack

Compilier warnings with 1.9.1

dilyanpalauzov opened this issue · 4 comments

dilyanpalauzov commented

With gcc 9.3.1 20200506 compiling mspack 1.9.1 prints:

mspack/chmd.c: In function 'chmd_extract':
mspack/chmd.c:1139:10: warning: 'length' may be used uninitialized in this function [-Wmaybe-uninitialized]
 1139 |   length -= self->d->offset;
      |          ^
mspack/chmd.c:1042:9: note: 'length' was declared here
 1042 |   off_t length, offset;
      |         ^
  CC       test/chmd_order.o
  CCLD     test/chmd_order
mspack/chmd.c: In function 'chmd_extract':
mspack/chmd.c:1139:10: warning: 'length' may be used uninitialized in this function [-Wmaybe-uninitialized]
 1139 |   length -= self->d->offset;
      |          ^
mspack/chmd.c:1042:9: note: 'length' was declared here
 1042 |   off_t length, offset;
      |         ^
  CC       test/chminfo.o
test/chminfo.c: In function ‘main’:
test/chminfo.c:255:47: warning: format ‘%llu’ expects argument of type ‘long long unsigned int’, but argument 4 has type ‘off_t’ {aka ‘long int’} 
[-Wformat=]
  255 |               printf("    %-10u -> %-10u [ %llu %u ]\n",
      |                                            ~~~^
      |                                               |
      |                                               long long unsigned int
      |                                            %lu
......
  258 |                      contents + rtdata,
      |                      ~~~~~~~~~~~~~~~~~         
      |                               |
      |                               off_t {aka long int}
  CCLD     test/chminfo
mspack/chmd.c: In function 'chmd_extract':
mspack/chmd.c:1139:10: warning: 'length' may be used uninitialized in this function [-Wmaybe-uninitialized]
 1139 |   length -= self->d->offset;
      |          ^
mspack/chmd.c:1042:9: note: 'length' was declared here
 1042 |   off_t length, offset;
      |         ^
make[1]: Target 'all-am' not remade because of errors.
kyz commented

Thanks for the output. I will take a look.

kyz commented

I took a look, it's a false positive.

  /* read the reset table entry */
  if (read_reset_table(self, sec, entry, &length, &offset)) {
    /* the uncompressed length given in the reset table is dishonest.
     * the uncompressed data is always padded out from the given
     * uncompressed length up to the next reset interval */
    length += reset_interval - 1;
    length &= -reset_interval;
  }

Firstly, read_reset_table() is called with a pointer to length. The code in read_reset_table() which sets length is this:

    /* get the uncompressed length of the LZX stream */
    if (read_off64(length_ptr, &data[lzxrt_UncompLen], sys, self->d->infh)) {
        sys->free(data);
        return 0;
    }

If data is not written to length_ptr, then read_reset_table() returns 0. All other checks that bail out of read_reset_table() before writing to length_ptr also return 0. So going back to the caller, if read_reset_table() returns non-zero then length has definitely been set. If read_reset_table() returns zero, the program enters this else clause, which calls read_spaninfo():

  else {
    /* if we can't read the reset table entry, just start from
     * the beginning. Use spaninfo to get the uncompressed length */
    entry = 0;
    offset = 0;
    err = read_spaninfo(self, sec, &length);
  }
  if (err) return self->error = err;

Now there's a call to read_spaninfo() which might set the length:

    /* find SpanInfo file */
    int err = find_sys_file(self, sec, &sec->spaninfo, spaninfo_name);
    if (err) return MSPACK_ERR_DATAFORMAT;

    /* check it's large enough */
    if (sec->spaninfo->length != 8) {
        D(("SpanInfo file is wrong size"))
        return MSPACK_ERR_DATAFORMAT;
    }

    /* read the SpanInfo file */
    if (!(data = read_sys_file(self, sec->spaninfo))) {
        D(("can't read SpanInfo file"))
        return self->error;
    }

    /* get the uncompressed length of the LZX stream */
    err = read_off64(length_ptr, data, sys, self->d->infh);
    sys->free(data);
    if (err) return MSPACK_ERR_DATAFORMAT;
  1. if find_sys_file() fails or the file length is not 8, it returns a non-zero error and doesn't set length
  2. if read_sys_file() fails, it will return self->error and doesn't set length; read_sys_file() sets self->error to a non-zero value in all cases where it returns failure, so there is not a case where it returns failure but self->error is zero
  3. if read_off64() fails, it will return a non-zero error and might not have set the length
  4. if read_off64() completes without error, length has been set. so if read_spaninfo() returns zero, length has been set

The next line after read_spaninfo() returns an error if one has been set:

  if (err) return self->error = err;

Therefore, length -= self->d->offset is never reached without length first being set. The warning is a false positive.

kyz commented

The second warning is that the format string is "%llu" aka long long unsigned int but on your system, off_t is merely long int.

The format string uses the macro LU" defined in mspack/macros.h. Leaving aside that this should be LD` (because the off_t expression is signed), the crux of the matter is that "long int" vs "long long int" are different sizes on your system.

The code in mspack/macros.h is supposed to cover this:

/* define LD and LU as printf-format for signed and unsigned long offsets */
#if HAVE_INTTYPES_H
# include <inttypes.h>
#else
# define PRId64 "lld"
# define PRIu64 "llu"
# define PRId32 "ld"
# define PRIu32 "lu"
#endif

#if SIZEOF_OFF_T >= 8
# define LD PRId64
# define LU PRIu64
#else
# define LD PRId32
# define LU PRIu32
#endif

PRId64, PRIu64, PRId32, PRIu32 are defined in C99, which this codebase doesn't require but will take advantage of. If you don't have C99's <inttypes.h> then libmspack makes a best guess.

Then there's a conditional definition. If your off_t has a size of 8 bytes or more (64-bit), then it should be printed with PRId64. If it's less than 8 bytes, PRId32.

If you want to debug this any further, it really depends on what your environment has actually set, e.g. something like:

gcc -I. -Imspack -DHAVE_CONFIG_H -E test/chminfo.c
gcc -I. -Imspack -DHAVE_CONFIG_H -E -dM test/chminfo.c

... to dump both what files are read in, and what value defines have. You could also add in extra defines to see what branch was taken by the preprocessor.

What you might see is that off_t aka long int is 8 bytes (64-bit), long long int is 16 bytes, and you have no <inttypes.h> so the best-effort fallback is "lld", whereas if you had <inttypes.h> it would have declared PRId32 as "d" and PRId64 as "ld". But this is just a guess.

For an Ubuntu 22.04 system with x86_64 architecture, off_t defined as long int and PRId64 is defined as ld, so they fit together:

$ cat ldtest.c
#include <inttypes.h>
#include <stdio.h>
int main() {
    int size = (int) sizeof(off_t);
    printf("PRId64=%s PRId32=%s sizeof(off_t)=%d\n", PRId64, PRId32, size);
    return 0;
}
$ gcc -o ldtest ldtest.c
$ ./ldtest
PRId64=ld PRId32=d sizeof(off_t)=8
kyz commented

The warnings have been fixed to my satisfaction in 0d13d86, unconditionally writing the length parameter just before gcc's analysis fails and issues the false warning.

I don't have a solution for finding the perfect printf formatting macro that exactly matches the off_t type, but I've checked on a variety of environments and most of them get it right with the conditional definitions in macros.h. I'm closing this for now, but if you want to continue diagnosing it then please reopen the issue.