> From: splint-discuss-boun...@cs.virginia.edu
> [mailto:splint-discuss-boun...@cs.virginia.edu] On Behalf Of Wenzel, Bodo
> Sent: Monday, 20 February, 2012 03:27
> Splint assumes that pointers without annotation are the only reference to
> their memory spaces. By assigning a new address, this old pointer is lost.
> If annotated by "dependent" these pointers are not responsible for the
> memory.
Annotating the min_string pointer as dependent in fill_strings() is incorrect.
(That is, it will suppress the Splint diagnostic, but it makes an invalid claim
about how the pointer is used.)
options->min_string *is* the owner of any area it points to. Before min_string
is assigned, any such area should be released. What's needed is simply
free(options->min-string)
before assigning to it. That's safe in this program (or will be once the other
error is fixed) because options is correctly initialized after allocation, in
main(), with:
*options = options_type0;
The rvalue there is a static const options_type object; since it's static, it's
implicitly initialized as if it had an initializer of {0}, so all pointer-type
fields in the structure are set to null. And a null pointer may be safely
passed to free().
Note, though, that the initialization is only correct for the first element of
options, which is actually an array of 200 options_type structures. The code in
main() needs to be fixed to initialize all of the elements in the array (and
200 shouldn't appear in the code, since it needs to be used at least twice -
once for allocation and once for initialization).
>> Kept storage min_string passed as only param: free (min_string) storage
>> is transferred to a non-temporary reference after being passed as keep
>> parameter. The storage may be released or new aliases created. (Use
>> -kepttrans to inhibit warning)
> Here Splint found an actual error which is probably the root of your
> crashes.
Right. An area passed to free() cannot subsequently be used without invoking
undefined behavior. free(min_string) at the end of fill_strings() is simply
wrong.
> Splint does not report this error after annotating "options->min_string"
> any more, unfortunately.
Because the annotation is incorrect; with it you're lying to Splint.
> To get no reports at all, I had to add an "unused" annotation for "argc"
> and to release the memory of "charset".
Yes. I'm suspicious of the low_charset field anyway, but there's no indication
in the code we have of what it's being used for.
Another error in the code:
static const char lowercase[] = "alphaomega";
...
memcpy(charset, lowercase, 26);
You've just read past the end of the lowercase object: undefined behavior.
Some other comments:
- calloc() is always the wrong choice. It's one of those functions in the C
standard library that exists for historical reasons but provides no advantage
over other library functions, and has disadvantages. calloc() has two formal
parameters of identical type, so it's easy to get the actual parameters in the
wrong order, which doesn't affect program execution but can lead to errors in
maintenance. calloc() touches the entire allocated area, which can lead to
unnecessary page committing and cache cooling. It initializes storage with
all-bits-zero, which may be incorrect for floating-point and pointer types. The
multiplication to determine the size of the allocated area is done in library
code, so the compiler doesn't have an opportunity to warn about some possible
overflow errors. Instead of calloc(), use malloc() and initialize where
appropriate.
- sizeof(char) is always 1, by definition, regardless of the number of bits in
the char type. C measures objects in char-type storage units.
- atoi() is almost always the wrong choice. It returns -1 for any error -
overflow or an invalid string. There's no way to diagnose why it failed. And
casting the result of atoi() to a size_t is dangerous: you now have an integer
overflow vulnerability. Use strtoul(), check what it sets "end" to, and check
the result to make sure it's in range.
- The style of the code is inconsistent. For example, in some places pointers
are explicitly compared against NULL, while in others the comparison is
implicit. I realize this is just a cut-down, so we can't tell whether this is a
problem in the original source.
--
Michael Wojcik
Technology Specialist, Micro Focus
This message has been scanned by MailController - portal1.mailcontroller.co.uk
_______________________________________________
splint-discuss mailing list
splint-discuss@mail.cs.virginia.edu
http://www.cs.virginia.edu/mailman/listinfo/splint-discuss
