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

3C introduces compile error when program element has unchecked declaration in one TU and itype in another TU #507

Open mattmccutchen-cci opened 3 years ago

mattmccutchen-cci commented 3 years ago

A problem affecting the icecast benchmark, from https://github.com/correctcomputation/actions/pull/13#pullrequestreview-618756386:

One of the errors I see in the test output is happening because file doesn't include stdlib.h. The file uses an implicit declaration of malloc. The implicit declaration doesn't have an itype, so it's an error for us to add a type argument. Do you think that this is something that could be addressed by Microsoft with their changes to the headers? Maybe implicit declaration of library function should use their checked prototypes.

Regardless of if Microsoft wants to address implicit library function declarations, I think that does expose and problem in 3C. We assume that a function has a single prototype across all translation units, but it looks like a function can be unchecked in one translation while having a checked prototype in another.

kyleheadley commented 3 years ago

Is this still an issue? I searched the "Build converted Icecast" report and it had no instances of "malloc". I know there's been a lot of itype improvements since this was posted.

john-h-kastner commented 3 years ago

The specific error we saw in icecast seems to be fixed, but a related issues persists. For two files foo.c and bar.c converted with 3c -output-dir=checked:

foo.c:

#include <stdlib.h>
void a() {
  int *b = malloc(sizeof(int));
}

bar.c:

void c() {
  int *d = malloc(sizeof(int));
}

3C doesn't add the type argument to malloc in bar.c, but it still treats the call as checked. This is an error, because a checked prototype for malloc doesn't exist in the translation unit for bar.c. Compiling the following converted bar.cgives an error.

void c() {
  _Ptr<int> d = malloc(sizeof(int));
}
bar.c:2:17: error: expression has unknown bounds, cast to ptr<T> expects source to have bounds
  _Ptr<int> d = malloc(sizeof(int));
                ^~~~~~~~~~~~~~~~~~~

More generally, we can trigger this error without using implicit declarations and special cased allocator functions.

foo.c:

int *test(void) : itype(_Ptr<int>);
void a() {
  int *b = test();
}

bar.c:

int *test();
void c() {
  int *d = test();
}
mattmccutchen-cci commented 3 years ago

Assuming we reuse this GitHub issue to refer to the issue John mentioned, if it isn't causing a benchmark error as far as we know, I think we should remove the label now. We can always add the label back if we rediscover the issue during error triage.

To the merits of the issue: I presume the cause is just that 3C links the test extern function across translation units and introduces a single atom for its return type, and in this case, for whatever reason, the "checked" status is taking priority and being used in bar.c? As long as 3C links global definitions, I don't think we can expect it to cope with arbitrary differences in declarations across translation units, and I'd be opposed to adding workarounds in narrower cases unless we have evidence that it's important in real-world code. Of course, we have a proposal in #341 to change the linking. John, what do you think? Is this issue worth keeping open?

john-h-kastner commented 3 years ago

I think this can be closed. I see two ways this might get fixed, and both are covered by existing issues.

mattmccutchen-cci commented 3 years ago

I see: this is not an arbitrary difference in declarations, but rather one in which the declarations would be compatible if they appeared in the same translation unit. Yes, it would be nice if 3C supported that. At this point, I can't rule out that there might be similar cases not covered by #678, but I think we can wait until we run into them to file an issue. Thanks.

mattmccutchen-cci commented 3 years ago

Actually, I thought of at least one case that #678 (as currently stated) wouldn't fix: if the declaration without the itype is #included from unwritable code. So I think we should keep this issue for all cases not covered by #678, though it may be low priority and we may decide that there isn't any solution whose benefit would justify the implementation effort. I doubt we'd want 3C to automatically add a redeclaration with the itype to the user's file, but perhaps 3C could issue a warning to help the user understand the root cause of the compile error.

mattmccutchen-cci commented 3 years ago

A similar problem with environ came up in #725, so I'm updating the title of this issue to cover all program elements that can have itypes, not just functions.