[Fortran] Fix some issues found by Coverity's static-code analysis scan

Dear all,

this patch fixes some of the warning showing up for gcc/fortran at 
http://scan.coverity.com/. Coverity sells static C/C++/C#/Java code 
analyzers and offer scanning to open-source projects. The result looks 
like 
http://www.coverity.com/images/products/static-analysis/screenshot-1-large.png

For GCC with a C,C++,Fortran,LTO build it shows 12,000 "defects", many 
are false positives, e.g. for gcc/fortran three times a warning that
   default:
      gcc_unreachable();
is unreachable as all cases have been covered. If they were covered via 
"case", one could remove the default and let -Wswitch -Werror do the 
work. But in those examples, there are also some additional "if"  
involved, which are before the "switch" - only with those the "default" 
is unreachable.Thus, the "default" has to stay.

The new version of scan.coverity.com allows the project to upload the 
builds themselves - which is the reason that now also Fortran is 
supported. I done a manual build and uploaded it; it could be 
automatized, but that only makes sense if someone regularly looks at the 
results. In any case, if you have already access to scan.coverity.com, 
feel free to log in and look at the results. If not, you can create a 
new account. (I don't know whether I can use the build-upload password 
to create accounts, whether someone else has a project account password 
or whether Coverity has to handle it themselves.)

I have now fixed some of the issues; please have a close look whether 
they make sense. With static analysis as with compiler warnings, one 
sometimes tends to fix them bliendly - causing wrong-code issues that 
way. I also tried to only fix code which is either wrong or bad style; I 
have ignore more bogus errors. Doing so, I have also added some 
gcc_asserts; one could decide that those aren't needed and if a NULL 
derference ever occurs, segfaulting (with the new libbacktrace stack 
trace) gives is sufficient feedback.

Overview about the changes:

* [Memory leak] arith.c's arith_power: We have "result = 
gfc_get_constant_expr (...);", which has to be freed in case of error - 
which we did only in one error case. For some reason, the scanner found 
only the second problem in BT_COMPLEX but not the one in BT_REAL.

* [Out of bounds] array.c's gfc_match_array_ref: I am not sure whether 
that code is unreachable, but in any case it is out of bounds. It does 
not seem to trigger for our test cases; maybe removing it and adding an 
assert is sufficient?

* [Uninitialized memory] array.c's gfc_match_array_constructor: 
gfc_match_decl_type_spec checks whether "ts->kind == -1" thus we to 
initialize it - either directly or as I did with the sledgehammer.

* [dereferenced + later NULL check] array.c's 
gfc_resolve_character_array_constructor: The code has:

   if (expr->ts.u.cl == NULL)
       expr->ts.u.cl = gfc_new_charlen (gfc_current_ns, NULL);

   if (expr->ts.u.cl->length == NULL)
   else
       gcc_assert (expr->ts.u.cl->length);

As "...->cl->length" is dereferenced, doesn't make sense to check again 
whether "...->cl" is NULL. (Besides, the code flow shows that it shouldn't.)

* [dereferenced + later NULL check] array.c's gfc_array_dimen_size: 
Similar issue, but this time I added a gcc_assert.

* [Same check twice] check.c's numeric_check: Here, one firsts returns 
if the type is numeric, if not, one tries implicit typing. I think the 
second EXPR_VARIABLE should have been EXPR_FUNCTION.

* [dereferenced + later NULL check] class.c's gfc_build_class_symbol. We 
later (for "rank = " check whether "as" exists. I think all calls are of 
the form "&sym->as", but I still added an assert.

* [memory leak] class.c's finalize_component: The "e" is created as 
copy_expr with main component ref added. It is either used in the 
DEALLOCATE or FINAL call, or passed in the loop for subcomponent 
finalization - as those use gfc_copy_expr, it has to be freed in that case.

* [dereferenced + later NULL check] dump-parse-tree.c's show_namespace. 
I think the calls are all pointing to non-NULL namespace, but to be 
careful, I added an assert and removed the check. (Most changes are pure 
re-indenting.)

* [Memory leak]  trans-io.c's transfer_namelist_element: One had code like

   if (ts->type == BT_DERIVED)
        tree expr = build_fold_indirect_ref_loc (input_location,
        for (cmp = ts->u.derived->components; cmp; cmp = cmp->next)
           transfer_namelist_element (block,
                                      full_name,
                                      NULL, cmp, expr);

However, when the derived type has no components, the "expr" is leaking. 
As Fortran 2003 allows empty components, that can actually occur in reality.

* [Memory leak]  trans-io.c's transfer_expr: The same issue.

* [Uninitialized memory] trans-io.c's gfc_trans_transfer: Here, the 
warning is about accessing uninitialized loop elements in 
gfc_cleanup_loop. The reason is that those aren't initialized for 
expr->rank == 0; I thought that the code should be unreachable due to 
the (se.ss != 0) condition, but an assert fails for instance for 
graphite/pr36286.f90. On the other hand, for se.ss != NULL && expr->rank 
== 0, handling lower code part is wrong as it only deals with "loop". 
Probably, one needs to use "if (se.ss == NULL || expr->rank == 0)", 
which I now did.

* [NULL-pointer access?] trans-io.c's gfc_trans_transfer: The issue 
should not occur, but to be safe and to silence the warning. The problem 
the scanner sees is that "ref" can be NULL if there is no ref->type == 
REF_ARRAY.

* [Memory leakage] trans.c's gfc_trans_runtime_check: We forget to call 
va_end.

I think that covers about 1/3 of the gcc/fortran warnings. By the way, 
there are no libgfortran warnings. Most of the remaining issues are 
about memory leakages (or bogus), but for the memory-leak ones require a 
closer look as the analysis jumps through the whole code.

TODO:

There are also real issues like the following in symbol.c's 
gfc_undo_symbols:

               gfc_free_namelist (old->namelist_tail);
               old->namelist_tail->next = NULL;

where one first frees the memory and then sets a component to NULL. 
Locally, I was wondering whether the two lines should be swapped as 
free_namelist also frees ns->next. Alternatively, the "->next" part is 
wrong, which is more in line with the next line of code:
       p->namelist_tail = old->namelist_tail;
But I think one needs to study the code more closely to know what the 
code is supposed to do.

(Unfortunately, there does not seem to be a match for module.c. I know 
that there are memory leaks but the code is written such that one has no 
idea about the flow - especially as a lot is done via "void *" pointers.)

Build and regtested on x86-64-linux.
OK for the trunk?

Tobias

[Fortran] Fix some issues found by Coverity's static-code analysis scan

Commit Message

Comments

Patch