Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for malloc incorrectly preventing checked region addition (issue #486) #527

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

aaronjeline
Copy link
Collaborator

#include <stdlib.h>
int *getarr(unsigned z) {
  int *s = malloc(sizeof(int)*z);
  return s;
}

now produces the following when run with -alltypes -addcr:

#include <stdlib.h>
_Array_ptr<int> getarr(unsigned z) : count(z) _Checked {
  _Array_ptr<int> s : count(z) = malloc<int>(sizeof(int)*z);
  return s;
}

This patch also revealed some other soundness issues with checked region insertion that have been fixed.
It also introduces a minor regression, which is that calling a function with ityped arguments will cause a region to be inferred as Unchecked. Will open a second issue for this because it will never cause compilation issues and I know this fix was high priority.

Copy link
Member

@mwhicks1 mwhicks1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some questions about the tests, code.

@@ -18,15 +18,15 @@ void test_none() {
int *i = 0;
foo(i);
}
// CHECK: void test_none() _Checked {
// CHECK: void test_none() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this not have been checked? If not, why wouldn't have the compilation by clang, post conversion, discovered the issue? Perhaps the test was not doing that part?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is a general question: It would be nice to confirm that the added annotation makes the Checked C compiler happy, so we should make sure that these tests are doing that, for -addcr runs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This happens because of the regression I mentioned above. It's fine to call a function who's parameters are unchecked but have itypes in a checked region, but the change I made to fix one issue revealed this one. If it was an issue that caused miscompilation we would have caught it, and in fact the tests did catch such an issue. Working on fixing this now but I though you might want this change in sooner because of the example code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in latest commit

@@ -39,7 +39,7 @@ int vsf_sysdep_has_capabilities_as_non_root(void) {
static void do_sanity_checks(void) {
//CHECK: static void do_sanity_checks(void) _Checked {
{
//CHECK: {
//CHECK: _Unchecked {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this _Unchecked because of an unsafe external call, e.g., die ? Just curious if you know the reason.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of a call to a function with unchecked types (and itypes) is incorrectly inferred as being unchecked, as above

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in latest commit

clang/lib/3C/CheckedRegions.cpp Outdated Show resolved Hide resolved
if (Cv.hasValue())
return Cv.getValue().hasWild(E) || Cv.getValue().hasParamWild(E);
else
return false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering where the presence of type instantiations (i.e., that you return malloc<int>(...) and not malloc(...)) is showing up?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 227 in the handling of call expressions

@mattmccutchen-cci mattmccutchen-cci changed the title Fix for Issue 486 Fix for malloc incorrectly preventing checked region addition (issue #486) Apr 5, 2021
@mattmccutchen-cci
Copy link
Member

As we look over the PR list, it would be helpful to have titles more descriptive than "Fix for issue #N". @aaronjeline Is the new title I gave to this PR reasonable?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants