[splint-discuss] Annoying False Positives

[Richard A. O'Keefe]

"Felipe Contreras" <felipe.contreras at gmail.com> asked:
	I have stumbled upon a lot of false positive errors with splint.
	
	Here is the one that bothers me the most (attached).
	
When I run that through splint it reports two errors.
Neither of them is a false positive.

home/users/okeefe_r/quasar/sp-test.c:22:2:
    Only storage test->data (type void *) not released before assignment:
    test->data = malloc(100)
  A memory leak has been detected. Only-qualified storage is not released
  before the last reference to it is lost. (Use -mustfreeonly to inhibit
  warning)

That doesn't refer to, but is caused by the call to memset().


    /*@null@*/ static MyTest *
    my_test_new(void) {
	MyTest *test;
	test = (MyTest *) malloc (sizeof (MyTest));

	if (!test) { return NULL; }
>>>	memset (test, 0, sizeof (MyTest))
	test->data = malloc (100);

	return test;
    }

splint is a checker for the ANSI C89/ISO C90 language.
That language gives us ***NO*** reason to believe that a word
with all bytes 0 is a null pointer, and in fact there have been
C implementations where the null pointer WASN'T all bytes 0.
Using memset to initialise a struct is in general such a daft idea
that splint really ought to complain about this on general principles.
The cleanest and simplest way I know to initialise a struct to something
sane is just

    static MyTest const zero_test;

and then

    test = zero_test;

Yes, I know that null pointer constants in C are *spelled* 0 in the
*source* code, but that has no implications whatever for what the bytes
will be in memory.

Anyway, here splint sees a struct variable that is initialised to
who knows what.  *It* certainly doesn't know what the variable is
initialised to (and if it did figure out that all bytes were being
set to zero, it has as pointed out above NO reason to believe that
all bytes zero is NOT a valid pointer to some object).

Simply remove that line and that message goes away.

The second message is this:
home/users/okeefe_r/quasar/sp-test.c:24:9:
    Returned storage *test contains 1 undefined field: data
  Storage derivable from a parameter, return value or global is not defined.
  Use /*@out@*/ to denote passed or returned storage which need not be defined.
  (Use -compdef to inhibit warning)

I believe this is referring to the fact that while
test is defined, and, if test is not null,
test->data is defined,
if test->data is not null, then the contents of test->data are NOT defined.

By the way, it is bad style to test pointers using "!".

Rewrite the function like this:

    /*@null@*/ static MyTest *
    my_test_new(void) {
	MyTest *test = malloc(sizeof *test);
	size_t const n = 100;

	if (test == NULL) return test;
	test->data = malloc(n);
	if (test->data == NULL) return test;
	memset(test->data, 0, n);
	return test;
    }

Hmm.  While rewriting like that is a good idea, it still doesn't make
the report go away.  Think think.  Ah!

Why do you ever want to return a non-NULL pointer to a record containing
a NULL data pointer?  I mean, the data pointer gets to be NULL only if
you just ran out of memory; what are the odds that you will be able to do
much of _anything_ else afterwards?  If you change the specification of
the function to

    my_test() either returns a pointer to a MyTest record containing
    a pointer to a zeroed block of 100 bytes, or returns NULL, indicating
    that memory ran out.

    /*@null@*/ static MyTest *
    my_test_new(void) {
	MyTest *test = malloc(sizeof *test);
	size_t const n = 100;

	if (test == NULL) return NULL;
	test->data = malloc(n);
	if (test->data == NULL) { free(test); return NULL; }
	memset(test->data, 0, n);
	return test;
    }

With that definition, there are no more complaints from splint.