Skip to content

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

Merged
mgrang merged 4 commits into
masterfrom
dyn-check2
Aug 20, 2019
Merged

mgrang merged 4 commits into
masterfrom
dyn-check2

Conversation

@mgrang

@mgrang mgrang commented Aug 14, 2019

Copy link
Copy Markdown

No description provided.

@mgrang

mgrang commented Aug 14, 2019

Copy link
Copy Markdown
Author

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

@mgrang

mgrang commented Aug 15, 2019

Copy link
Copy Markdown
Author

This patch uncovered a bug in checkedc-parson: https://github.com/microsoft/checkedc-parson/pull/18

@dtarditi dtarditi left a comment

Copy link
Copy Markdown
Member

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.

Comment thread lib/CodeGen/CGDynamicCheck.cpp Outdated
++NumDynamicChecksNonNull;

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

Copy link
Copy Markdown
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.


// 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
Copy Markdown
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

mgrang commented Aug 19, 2019

Copy link
Copy Markdown
Author

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

@dtarditi dtarditi left a comment

Copy link
Copy Markdown
Member

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

mgrang commented Aug 20, 2019

Copy link
Copy Markdown
Author

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