[SYCL][NFC] Fix static code analysis concerns #2789

smanna12 · 2020-11-18T08:28:07Z

Found via a static-analysis tool:

Inside leaveStruct()

RecordDecl *RD = Ty->getAsRecordDecl(); ---> Pointer 'RD' may be NULL
if (!RD->hasAttr()) ----> will be dereferenced here

Inside visitRecord() function, Pointer 'RecordTy->getAsRecordDecl()' returned from
call to function 'getAsRecordDecl' at line
“if (RecordTy->getAsRecordDecl()->hasAttr()) {“
may be NULL and will be dereferenced at same line.

This patch fixes null pointer dereference issues in SemaSYCL.cpp file.

Signed-off-by: Soumi Manna [email protected]

Found via a static-analysis tool: 1. Inside visitRecord() Pointer 'RecordTy->getAsRecordDecl()' returned from call to function 'getAsRecordDecl' may be NULL and will be dereferenced at line below: “if (RecordTy->getAsRecordDecl()->hasAttr<SYCLRequiresDecompositionAttr>()) {“ 2. Inside leaveStruct() RecordDecl *RD = Ty->getAsRecordDecl(); --->Pointer 'RD' may be NULL if (!RD->hasAttr<SYCLRequiresDecompositionAttr>()) ---> will be dereferenced here 3. Inside leaveStruct() RecordDecl *RD = Ty->getAsRecordDecl(); ---> Pointer 'RD' may be NULL if (!RD->hasAttr<SYCLRequiresDecompositionAttr>()) ----> will be dereferenced here 4. Inside handleSyclAccessorType() const auto *AccTy = cast<ClassTemplateSpecializationDecl>(FieldTy->getAsRecordDecl()); ---> Pointer 'AccTy' may be NULL assert(AccTy->getTemplateArgs().size() >= 2 && ----> will be dereferenced here "Incorrect template args for Accessor Type"); 5. Inside handleSyclAccessorType() const auto *AccTy = cast<ClassTemplateSpecializationDecl>(FieldTy->getAsRecordDecl()); ---> Pointer 'AccTy' may be NULL assert(AccTy->getTemplateArgs().size() >= 2 &&  will be dereferenced here "Incorrect template args for Accessor Type"); 6. Inside checkBufferLocationType() const auto *PropDecl = cast<ClassTemplateSpecializationDecl>(PropTy->getAsRecordDecl()); -->Pointer 'PropDecl' may be NULL if (PropDecl->getTemplateArgs().size() != 1) {---> will be dereferenced here This patch fixes null pointer dereference issues in SemaDecl.cpp file. Signed-off-by: Soumi Manna <[email protected]>

Signed-off-by: Soumi Manna <[email protected]>

Fznamznon · 2020-11-18T10:46:32Z

clang/lib/Sema/SemaSYCL.cpp

+  if (RecordTy->getAsRecordDecl() != nullptr &&
+      RecordTy->getAsRecordDecl()->hasAttr<SYCLRequiresDecompositionAttr>()) {


I suggest calling getAsRecordDecl() once.

Suggested change

if (RecordTy->getAsRecordDecl() != nullptr &&

RecordTy->getAsRecordDecl()->hasAttr<SYCLRequiresDecompositionAttr>()) {

RecordDecl *RD = RecordTy->getAsRecordDecl();

if (RD && RD->hasAttr<SYCLRequiresDecompositionAttr>()) {

Fznamznon · 2020-11-18T10:47:30Z

clang/lib/Sema/SemaSYCL.cpp

@@ -1351,7 +1352,7 @@ class SyclKernelFieldChecker : public SyclKernelFieldHandler {
  void checkBufferLocationType(QualType PropTy, SourceLocation Loc) {
    const auto *PropDecl =
        cast<ClassTemplateSpecializationDecl>(PropTy->getAsRecordDecl());
-    if (PropDecl->getTemplateArgs().size() != 1) {
+    if (PropDecl != nullptr && PropDecl->getTemplateArgs().size() != 1) {


We can make it simpler

Suggested change

if (PropDecl != nullptr && PropDecl->getTemplateArgs().size() != 1) {

if (PropDecl && PropDecl->getTemplateArgs().size() != 1) {

Fznamznon · 2020-11-18T10:47:59Z

clang/lib/Sema/SemaSYCL.cpp

@@ -1583,7 +1584,7 @@ class SyclKernelDecompMarker : public SyclKernelFieldHandler {
  bool leaveStruct(const CXXRecordDecl *, FieldDecl *, QualType Ty) final {
    if (CollectionStack.pop_back_val()) {
      RecordDecl *RD = Ty->getAsRecordDecl();
-      if (!RD->hasAttr<SYCLRequiresDecompositionAttr>())
+      if (RD != nullptr && !RD->hasAttr<SYCLRequiresDecompositionAttr>())


Same here

Suggested change

if (RD != nullptr && !RD->hasAttr<SYCLRequiresDecompositionAttr>())

if (RD && !RD->hasAttr<SYCLRequiresDecompositionAttr>())

Fznamznon · 2020-11-18T10:48:20Z

clang/lib/Sema/SemaSYCL.cpp

@@ -1601,7 +1602,7 @@ class SyclKernelDecompMarker : public SyclKernelFieldHandler {
                   QualType Ty) final {
    if (CollectionStack.pop_back_val()) {
      RecordDecl *RD = Ty->getAsRecordDecl();
-      if (!RD->hasAttr<SYCLRequiresDecompositionAttr>())
+      if (RD != nullptr && !RD->hasAttr<SYCLRequiresDecompositionAttr>())


Same here

Suggested change

if (RD != nullptr && !RD->hasAttr<SYCLRequiresDecompositionAttr>())

if (RD && !RD->hasAttr<SYCLRequiresDecompositionAttr>())

Signed-off-by: Soumi Manna <[email protected]>

erichkeane

This patch changes control flow in REALLY bad error conditions, it should not.

erichkeane · 2020-11-18T14:07:32Z

clang/lib/Sema/SemaSYCL.cpp

@@ -1214,7 +1214,8 @@ void KernelObjVisitor::visitRecord(const CXXRecordDecl *Owner, ParentTy &Parent,
                                   const CXXRecordDecl *Wrapper,
                                   QualType RecordTy,
                                   HandlerTys &... Handlers) {
-  if (RecordTy->getAsRecordDecl()->hasAttr<SYCLRequiresDecompositionAttr>()) {
+  RecordDecl *RD = RecordTy->getAsRecordDecl();
+  if (RD && RD->hasAttr<SYCLRequiresDecompositionAttr>()) {


This should be an assert here, not a test. If we are here and RD is null, something very wrong has happened.

erichkeane · 2020-11-18T14:07:59Z

clang/lib/Sema/SemaSYCL.cpp

@@ -1351,7 +1352,7 @@ class SyclKernelFieldChecker : public SyclKernelFieldHandler {
  void checkBufferLocationType(QualType PropTy, SourceLocation Loc) {
    const auto *PropDecl =
        cast<ClassTemplateSpecializationDecl>(PropTy->getAsRecordDecl());
-    if (PropDecl->getTemplateArgs().size() != 1) {
+    if (PropDecl && PropDecl->getTemplateArgs().size() != 1) {


Should this be an assert as well?

erichkeane · 2020-11-18T14:08:11Z

clang/lib/Sema/SemaSYCL.cpp

@@ -1583,7 +1584,7 @@ class SyclKernelDecompMarker : public SyclKernelFieldHandler {
  bool leaveStruct(const CXXRecordDecl *, FieldDecl *, QualType Ty) final {
    if (CollectionStack.pop_back_val()) {
      RecordDecl *RD = Ty->getAsRecordDecl();
-      if (!RD->hasAttr<SYCLRequiresDecompositionAttr>())
+      if (RD && !RD->hasAttr<SYCLRequiresDecompositionAttr>())


This definitely should be an assert.

erichkeane · 2020-11-18T14:08:17Z

clang/lib/Sema/SemaSYCL.cpp

@@ -1601,7 +1602,7 @@ class SyclKernelDecompMarker : public SyclKernelFieldHandler {
                   QualType Ty) final {
    if (CollectionStack.pop_back_val()) {
      RecordDecl *RD = Ty->getAsRecordDecl();
-      if (!RD->hasAttr<SYCLRequiresDecompositionAttr>())
+      if (RD && !RD->hasAttr<SYCLRequiresDecompositionAttr>())


erichkeane · 2020-11-18T14:08:35Z

clang/lib/Sema/SemaSYCL.cpp

@@ -2703,6 +2704,8 @@ class SyclKernelIntHeaderCreator : public SyclKernelFieldHandler {
                              QualType FieldTy) final {
    const auto *AccTy =
        cast<ClassTemplateSpecializationDecl>(FieldTy->getAsRecordDecl());
+    if (!AccTy)


I don't think this is possible either, so this should assert.

erichkeane · 2020-11-18T14:08:41Z

clang/lib/Sema/SemaSYCL.cpp

@@ -2717,6 +2720,8 @@ class SyclKernelIntHeaderCreator : public SyclKernelFieldHandler {
  bool handleSyclAccessorType(FieldDecl *FD, QualType FieldTy) final {
    const auto *AccTy =
        cast<ClassTemplateSpecializationDecl>(FieldTy->getAsRecordDecl());
+    if (!AccTy)


assert here as well.

erichkeane

Should have done this before, making sure i 'request changes'.

Signed-off-by: Soumi Manna <[email protected]>

erichkeane

Sorry, my 1st review was before caffeine and I missed that these were the results of casts.

erichkeane · 2020-11-18T15:03:57Z

clang/lib/Sema/SemaSYCL.cpp

@@ -2720,8 +2723,7 @@ class SyclKernelIntHeaderCreator : public SyclKernelFieldHandler {
  bool handleSyclAccessorType(FieldDecl *FD, QualType FieldTy) final {
    const auto *AccTy =
        cast<ClassTemplateSpecializationDecl>(FieldTy->getAsRecordDecl());
-    if (!AccTy)
-      return false;
+    assert(AccTy && "should not be null.");


So actually upon reread, I think this is a false-positive. the 'cast' above should assert on null or wrong type, so this shouldn't be necessary.

Thanks @erichkeane for pointing this. I also missed the fact that we are using cast here.

erichkeane · 2020-11-18T15:04:01Z

clang/lib/Sema/SemaSYCL.cpp

@@ -2704,8 +2708,7 @@ class SyclKernelIntHeaderCreator : public SyclKernelFieldHandler {
                              QualType FieldTy) final {
    const auto *AccTy =
        cast<ClassTemplateSpecializationDecl>(FieldTy->getAsRecordDecl());
-    if (!AccTy)
-      return false;
+    assert(AccTy && "should not be null.");


erichkeane · 2020-11-18T15:04:09Z

clang/lib/Sema/SemaSYCL.cpp

@@ -1352,7 +1353,8 @@ class SyclKernelFieldChecker : public SyclKernelFieldHandler {
  void checkBufferLocationType(QualType PropTy, SourceLocation Loc) {
    const auto *PropDecl =
        cast<ClassTemplateSpecializationDecl>(PropTy->getAsRecordDecl());
-    if (PropDecl && PropDecl->getTemplateArgs().size() != 1) {
+    assert(PropDecl && "should not be null.");


erichkeane

I'll leave it to @Fznamznon and @elizabethandrews and @premanandrao to bikeshed assert messages if they want, but functionally this looks right.

smanna12 · 2020-11-18T16:20:47Z

Thank you everyone for the reviews.

smanna12 added 2 commits November 18, 2020 00:08

Fix Clang format issues

38564bc

Signed-off-by: Soumi Manna <[email protected]>

smanna12 marked this pull request as ready for review November 18, 2020 09:12

smanna12 requested review from elizabethandrews, Fznamznon and premanandrao as code owners November 18, 2020 09:12

smanna12 requested a review from erichkeane November 18, 2020 09:25

Fznamznon reviewed Nov 18, 2020

View reviewed changes

Address review comments

46e0fe7

Signed-off-by: Soumi Manna <[email protected]>

smanna12 requested a review from Fznamznon November 18, 2020 11:48

erichkeane reviewed Nov 18, 2020

View reviewed changes

erichkeane suggested changes Nov 18, 2020

View reviewed changes

smanna12 added 2 commits November 18, 2020 06:49

update patch based on review comments

3ecd025

Signed-off-by: Soumi Manna <[email protected]>

update patch

9c6e9a5

Signed-off-by: Soumi Manna <[email protected]>

smanna12 requested a review from erichkeane November 18, 2020 15:02

erichkeane reviewed Nov 18, 2020

View reviewed changes

erichkeane approved these changes Nov 18, 2020

View reviewed changes

Fznamznon approved these changes Nov 18, 2020

View reviewed changes

elizabethandrews approved these changes Nov 18, 2020

View reviewed changes

premanandrao approved these changes Nov 18, 2020

View reviewed changes

bader merged commit e1d8db7 into intel:sycl Nov 19, 2020

		if (RecordTy->getAsRecordDecl() != nullptr &&
		RecordTy->getAsRecordDecl()->hasAttr<SYCLRequiresDecompositionAttr>()) {

	if (PropDecl != nullptr && PropDecl->getTemplateArgs().size() != 1) {
	if (PropDecl && PropDecl->getTemplateArgs().size() != 1) {

	if (RD != nullptr && !RD->hasAttr<SYCLRequiresDecompositionAttr>())
	if (RD && !RD->hasAttr<SYCLRequiresDecompositionAttr>())

[SYCL][NFC] Fix static code analysis concerns #2789

[SYCL][NFC] Fix static code analysis concerns #2789

Uh oh!

Conversation

smanna12 commented Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

smanna12 commented Nov 18, 2020

Uh oh!

Uh oh!

smanna12 commented Nov 18, 2020 •

edited

Loading