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

Add a dynamic check for pointer arithmetic of null pointers #663

Merged
merged 4 commits into from
Aug 20, 2019

Conversation

mgrang
Copy link

@mgrang mgrang commented Aug 14, 2019

No description provided.

@mgrang
Copy link
Author

mgrang commented Aug 14, 2019

This is a cleaned-up version of #647 with all git merge conflicts resolved.

mgrang pushed a commit to microsoft/checkedc-parson that referenced this pull request Aug 15, 2019
Pointer arithmetic was done before checking for null.  This bug is uncovered by
inserting dynamic null ptr checks in
checkedc/checkedc-clang#663.
@mgrang
Copy link
Author

mgrang commented Aug 15, 2019

This patch uncovered a bug in checkedc-parson: microsoft/checkedc-parson#18

Copy link
Member

@dtarditi dtarditi left a comment

Choose a reason for hiding this comment

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

This change looks good. There appears to be a typo where a function is called twice.

Could you add dynamic tests for the Checked C repo that check the null pointer arithmetic properly fails? These tests should be placed in a new subdirectory of tests/dynamic_checking.

++NumDynamicChecksNonNull;

Value *ConditionVal = Builder.CreateIsNotNull(Val, "_Dynamic_check.non_null");
EmitDynamicCheckBlocks(ConditionVal); EmitDynamicCheckBlocks(ConditionVal);
Copy link
Member

Choose a reason for hiding this comment

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

It looks like EmitDynamicCheckBlocks is being called twice here.

@@ -1,6 +1,6 @@

// RUN: %clang_cc1 -fcheckedc-extension -verify -verify-ignore-unexpected=note %s -ast-dump | FileCheck %s --check-prefix=CHECK-AST
// RUN: %clang_cc1 -fcheckedc-extension -verify -verify-ignore-unexpected=note %s -emit-llvm -O0 -o - | FileCheck %s --check-prefix=CHECK-IR
// RUN: %clang_cc1 -fcheckedc-extension -fno-checkedc-null-ptr-arith -verify -verify-ignore-unexpected=note %s -emit-llvm -O0 -o - | FileCheck %s --check-prefix=CHECK-IR
Copy link
Author

Choose a reason for hiding this comment

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

By default, null ptr arithmetic checks are enabled. This breaks a few test cases which perform ptr arithmetic but which do not expect the checks. So I had to add the flag -fno-checkedc-null-ptr-arith to keep these tests from breaking.

@mgrang
Copy link
Author

mgrang commented Aug 19, 2019

The runtime tests for null ptr arithmetic have been added in checkedc/checkedc#378

Copy link
Member

@dtarditi dtarditi left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

Please open a GitHub issue for improving the tests. You and I discuss this in-person. We should update the codegen tests where we are currently disabling null-pointer arithmetic checks. In addition, the tests should cover local variables and parameters.

@mgrang
Copy link
Author

mgrang commented Aug 20, 2019

Looks great, thank you!

Please open a GitHub issue for improving the tests. You and I discuss this in-person. We should update the codegen tests where we are currently disabling null-pointer arithmetic checks. In addition, the tests should cover local variables and parameters.

Done #669.

@mgrang mgrang merged commit b242b14 into master Aug 20, 2019
@mgrang mgrang deleted the dyn-check2 branch August 20, 2019 22:57
mgrang pushed a commit that referenced this pull request Sep 27, 2019
Cherry-picked from commit b242b14

    * Add a dynamic check for pointer arithmetic of null pointers
Machiry pushed a commit to Machiry/checkedc-clang that referenced this pull request Jan 21, 2022
…crosoft-20210803

Merge from Microsoft 2021-08-03
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.

2 participants