correctcomputation / checkedc-clang

This is the primary development repository for 3C, a tool for automatically converting legacy C code to the Checked C extension of C, which aims to enforce spatial memory safety. This repository is a fork of Checked C's.
14 stars 5 forks source link

Unwritable function can get separate root causes for internal and external VarAtoms #681

Open mattmccutchen-cci opened 3 years ago

mattmccutchen-cci commented 3 years ago

If a function declared in an unwritable file with no body visible to 3C has a parameter or return value with a pointer type, 3C reports two root causes of wildness for the pointer: the external ConstraintVariable gets "Source code in non-writable file" and the internal ConstraintVariable gets "Unchecked pointer in parameter or return of external function". For a parameter, the source locations are different (the external variable is at the name of the function and the internal variable is at the parameter declaration), while for the return value, both source locations are at the name of the function; I haven't investigated why. This means that while PerWildPtrStats.json always has both root causes, -warn-root-cause will show only one of the two root causes for a return value because it runs separately on each translation unit and it generates at most one warning per PSL as a crude way to avoid reporting the same root cause multiple times in different translation units. (Maybe we should have a separate issue for -warn-root-cause to deduplicate root causes by VarAtom instead of by PSL?)

For a simple example, create clang/test/3C/root_cause_mini.c containing:

void my_extern(int *arg);

and clang/test/3C/base_subdir/unwritable_rootcauses_mini.c containing:

#include "../root_cause_mini.c"

and then run 3c -warn-all-root-cause unwritable_rootcauses_mini.c -- in clang/test/3C/base_subdir. The output:

In file included from /home/matt/3c-3.wt/clang/test/3C/base_subdir/unwritable_rootcauses_mini.c:1:
/home/matt/3c-3.wt/clang/test/3C/root_cause_mini.c:1:6: warning: Root cause for 0 unchecked pointers: Source code in non-writable file.
void my_extern(int *arg);
     ^
/home/matt/3c-3.wt/clang/test/3C/root_cause_mini.c:1:21: warning: Root cause for 0 unchecked pointers: Unchecked pointer in parameter or return of external function my_extern
void my_extern(int *arg);
                    ^
#include "../root_cause_mini.c"

Somehow, we didn't think to try this exact case in the existing unwritable_rootcauses.c test, or we would have noticed the strange behavior earlier. (The return value of glob_f does trigger the problem, but it is hidden by the PSL-based deduplication of -warn-root-cause.)

I'm finding the two root causes somewhat distracting when looking at the root-cause list in the Icecast port. I know that we'd like 3C to be able to record multiple root causes of the same pointer (#298), but we don't necessarily want our tools to display all of them to the user in all cases. Currently, I think displaying both "unwritable" and "extern" on different VarAtoms in a subset of cases (i.e., functions but not global variables) is more distracting than helpful.

From brief debugging, I think the phenomenon is caused by an unintended implementation inconsistency between equateWithItype (which seems to apply the "unwritable" constraint only to the external atom) and the additional code in ProgramInfo::link for functions with no body, which explicitly constrains both the internal and external atoms: https://github.com/correctcomputation/checkedc-clang/blob/0fd38a3b61eec76262d4d05b3e743a7f6660d60f/clang/lib/3C/ProgramInfo.cpp#L440-L444 See also #625 for another design concern about equateWithItype.

mattmccutchen-cci commented 3 years ago

I did some further testing. Here's a more complete example:

clang/test/3C/root_cause_mini.c:

void my_extern(int *arg);

void my_extern_with_itype(int *arg);
void my_extern_with_itype(int *arg : itype(_Ptr<int>));

typedef int *p_int;
void my_extern_with_typedef(p_int arg);

clang/test/3C/base_subdir/unwritable_rootcauses_mini.c:

#include "../root_cause_mini.c"

void foo(int *p) {
  my_extern(p);
}

void foo_with_typedef(int *p) {
  my_extern_with_typedef(p);
}

then the output is:

/home/matt/3c-3.wt/clang/test/3C/root_cause_mini.c:1:6: warning: Root cause for 1 unchecked pointer: Source code in non-writable file.
void my_extern(int *arg);
     ^
/home/matt/3c-3.wt/clang/test/3C/root_cause_mini.c:1:21: warning: Root cause for 0 unchecked pointers: Unchecked pointer in parameter or return of external function my_extern
void my_extern(int *arg);
                    ^
/home/matt/3c-3.wt/clang/test/3C/root_cause_mini.c:3:6: warning: Root cause for 0 unchecked pointers: Source code in non-writable file.
void my_extern_with_itype(int *arg);
     ^
/home/matt/3c-3.wt/clang/test/3C/root_cause_mini.c:4:6: warning: Root cause for 0 unchecked pointers: Source code in non-writable file.
void my_extern_with_itype(int *arg : itype(_Ptr<int>));
     ^
/home/matt/3c-3.wt/clang/test/3C/root_cause_mini.c:6:14: warning: Root cause for 1 unchecked pointer: Source code in non-writable file.
typedef int *p_int;
             ^
/home/matt/3c-3.wt/clang/test/3C/root_cause_mini.c:7:6: warning: Root cause for 1 unchecked pointer: Source code in non-writable file.
void my_extern_with_typedef(p_int arg);
     ^
/home/matt/3c-3.wt/clang/test/3C/root_cause_mini.c:7:35: warning: Root cause for 1 unchecked pointer: Unchecked pointer in parameter or return of external function my_extern_with_typedef
void my_extern_with_typedef(p_int arg);
                                  ^

Lessons:

  1. In the absence of typedefs, the number of affected pointers for the internal variable should always be 0. So if we look only at root causes with at least 1 affected pointer, the extra warning is less of a problem.
  2. When the parameter type is given by a typedef, then both the internal variable and the external variable show as having 1 affected pointer. This is probably why I saw the root causes for the internal variables as having high scores in Icecast. We plan to change the handling of typedefs (#389) at least in writable code, and it's possible that that will change the behavior here as well.
  3. It's possible to get separate "unwritable" root causes on the internal and external atoms if a parameter is first declared as fully unchecked and then redeclared with an itype. I'll broaden the issue title to cover this case.