Klocwork check null before dereference in src/acl_kernel.cpp #202

haoxian2 · 2022-11-17T03:07:13Z

Fixed the following Klocwork issues:

Pointer 'arg_value' checked for NULL at line 339 may be dereferenced at line 423.
Pointer 'arg_value' checked for NULL at line 345 may be dereferenced at line 423.
Pointer 'arg_value' checked for NULL at line 351 may be dereferenced at line 423.
Pointer 'arg_value' checked for NULL at line 412 may be dereferenced at line 423.

As pointed out by Peter, in the ACL_ARG_ADDR_NONE block, either the argument value is a sampler which must be non-null, or the argument value is a pipe which must be non-null, or the passed argument size matches the expected argument size, and the value must be non-null. Therefore, moving the NULL check to the front to ensure that we are not dereferencing a NULL value. Added an unit test check to check for NULL non-memory ARG.

Pointer 'pipe_ptr' checked for NULL at line 339 may be dereferenced at line 445.
Pointer 'sampler' checked for NULL at line 391 may be dereferenced at line 646.
Pointer 'sampler' checked for NULL at line 398 may be dereferenced at line 646.
Pointer 'sampler' checked for NULL at line 599 may be dereferenced at line 646.
Pointer 'sampler' checked for NULL at line 405 may be dereferenced at line 646.

acl_pipe_is_valid_pointer() and acl_sampler_is_valid_ptr() both returns 0 if arg_value is NULL, so that is_sampler and is_pipe won't be updated to TRUE if arg_value is NULL. Therefore adding assert for both pipe_ptr and sampler.

zibaiwan

Thanks for working on this. The changes look fine to me. Can you please squash the commits so we don't have the "clang format" commit at the end?

Ideally you should have 3 commits in total, one for each issue. You can use git rebase -i or other methods you prefer.

haoxian2 · 2022-11-17T19:11:13Z

Hi @haoxian2,

Thanks for working on this. The changes look fine to me. Can you please squash the commits so we don't have the "clang format" commit at the end?

Ideally you should have 3 commits in total, one for each issue. You can use git rebase -i or other methods you prefer.

@zibaiwan Made the change :)

pcolberg

Thanks @haoxian2! These should be returned as errors to the user, as detailed below.

src/acl_kernel.cpp

pcolberg

Thanks @haoxian2 for the revision, I think we are almost there. I needed multiple passes as well to notice the fine details. Please see if you can verify the NULL pointer checks using the unit test as detailed below.

src/acl_kernel.cpp

…r ensures that arg_value is not Null

…er_is_valid_ptr ensures pointer is not Null

pcolberg · 2022-11-18T20:39:17Z

As discussed offline with @haoxian2, this is pending on verifying that the test does indeed reach the desired code path(s).

haoxian2 · 2022-11-21T17:08:12Z

For the addition of assert(pipe_ptr) under the is_pipe block (line 437), this block can only be access if is_pipe is set to CL_TRUE. This can only happen in two occasions:

If addrspace is ACL_ARG_ADDR_GLOBAL or ACL_ARG_ADDR_CONSTANT (line 398). This block ensures that *(cl_mem *)arg_value is not NULL, therefore pipe_ptr = *(cl_mem *)arg_value cannot be NULL.
If addrspace is ACL_ARG_ADDR_NONE (line 426). This block uses acl_pipe_is_valid_pointer to return 0 if the pipe object is NULL, therefore also ensuring that the pipe object is not NULL.
Conclusion: is_pipe will not be set to CL_TRUE if pipe object is NULL.

pcolberg · 2022-11-21T20:58:36Z

Klocwork issues resolved in commit a38e1da:

---------------------------------------------------------------------------
14 (Local) /__w/fpga-runtime-for-opencl/fpga-runtime-for-opencl/test/acl_thread_test.cpp:143 NPD.GEN.CALL.MUST (1:Critical) Analyze
Null pointer '...?0:&val_arg' that comes from line 143 will be passed to function and can be dereferenced there by passing argument 4 to function 'clSetKernelArg' at line 143.
  * acl_thread_test.cpp:143: arg_info->addr_space==ACL_ARG_ADDR_LOCAL is true
  * acl_thread_test.cpp:143: '...?0:&val_arg' has been assigned a NULL value.
  * acl_thread_test.cpp:141: '...?0:&val_arg' is dereferenced by passing argument 4 to function 'clSetKernelArg'.
    * acl_kernel.cpp:725: 'arg_value' is passed to function 'clSetKernelArg'.
    * acl_kernel.cpp:726: 'arg_value' is dereferenced by passing argument 4 to function 'clSetKernelArgIntelFPGA'.
      * acl_kernel.cpp:316: 'arg_value' is passed to function 'clSetKernelArgIntelFPGA'.
      * acl_kernel.cpp:345: arg_value&& (arg_size!=sizeof(cl_sampler) || !acl_sampler_is_valid( * (cl_sampler* )arg_value) ) is false
      * acl_kernel.cpp:351: arg_value is false
      * acl_kernel.cpp:351: arg_size!=arg_info->size&&arg_value&&arg_size==sizeof(cl_sampler) &&acl_sampler_is_valid( * (cl_sampler* )arg_value) is false
      * acl_kernel.cpp:412: arg_value!=0 is false
      * acl_kernel.cpp:423: 'arg_value' is explicitly dereferenced.
Current status 'Analyze'

---------------------------------------------------------------------------
98 (Local) /__w/fpga-runtime-for-opencl/fpga-runtime-for-opencl/src/acl_kernel.cpp:423 NPD.CHECK.MIGHT (1:Critical) Analyze
Pointer 'arg_value' checked for NULL at line 339 may be dereferenced at line 423.
  * acl_kernel.cpp:336: arg_info->category==ACL_ARG_MEM_OBJ is true
  * acl_kernel.cpp:339: 'arg_value' is checked for NULL.
  * acl_kernel.cpp:423: 'arg_value' is explicitly dereferenced.
Current status 'Analyze'

---------------------------------------------------------------------------
99 (Local) /__w/fpga-runtime-for-opencl/fpga-runtime-for-opencl/src/acl_kernel.cpp:423 NPD.CHECK.MIGHT (1:Critical) Analyze
Pointer 'arg_value' checked for NULL at line 345 may be dereferenced at line 423.
  * acl_kernel.cpp:344: arg_info->category==ACL_ARG_SAMPLER is true
  * acl_kernel.cpp:345: 'arg_value' is checked for NULL.
  * acl_kernel.cpp:345: arg_value&& (arg_size!=sizeof(cl_sampler) || !acl_sampler_is_valid( * (cl_sampler* )arg_value) ) is false
  * acl_kernel.cpp:412: is_sampler is true
  * acl_kernel.cpp:412: arg_value!=0 is false
  * acl_kernel.cpp:423: 'arg_value' is explicitly dereferenced.
Current status 'Analyze'

---------------------------------------------------------------------------
100 (Local) /__w/fpga-runtime-for-opencl/fpga-runtime-for-opencl/src/acl_kernel.cpp:423 NPD.CHECK.MIGHT (1:Critical) Analyze
Pointer 'arg_value' checked for NULL at line 351 may be dereferenced at line 423.
  * acl_kernel.cpp:351: arg_size!=arg_info->size is true
  * acl_kernel.cpp:351: 'arg_value' is checked for NULL.
  * acl_kernel.cpp:351: arg_value is false
  * acl_kernel.cpp:351: arg_size!=arg_info->size&&arg_value&&arg_size==sizeof(cl_sampler) &&acl_sampler_is_valid( * (cl_sampler* )arg_value) is false
  * acl_kernel.cpp:423: 'arg_value' is explicitly dereferenced.
Current status 'Analyze'

---------------------------------------------------------------------------
101 (Local) /__w/fpga-runtime-for-opencl/fpga-runtime-for-opencl/src/acl_kernel.cpp:423 NPD.CHECK.MIGHT (1:Critical) Analyze
Pointer 'arg_value' checked for NULL at line 412 may be dereferenced at line 423.
  * acl_kernel.cpp:412: is_sampler is true
  * acl_kernel.cpp:412: 'arg_value' is checked for NULL.
  * acl_kernel.cpp:412: arg_value!=0 is false
  * acl_kernel.cpp:423: 'arg_value' is explicitly dereferenced.
Current status 'Analyze'

---------------------------------------------------------------------------
102 (Local) /__w/fpga-runtime-for-opencl/fpga-runtime-for-opencl/src/acl_kernel.cpp:445 NPD.CHECK.MIGHT (1:Critical) Analyze
Pointer 'pipe_ptr' checked for NULL at line 339 may be dereferenced at line 445.
  * acl_kernel.cpp:339: arg_value is true
  * acl_kernel.cpp:339: '*arg_value' is checked for NULL.
  * acl_kernel.cpp:339: * (cl_mem* )arg_value is false
  * acl_kernel.cpp:411: entering case statement arg_info->addr_space==ACL_ARG_ADDR_NONE
  * acl_kernel.cpp:412: is_sampler&&arg_value!=0&&acl_sampler_is_valid_ptr( * ( (cl_sampler* )arg_value) ) is false
  * acl_kernel.cpp:422: arg_size==sizeof(cl_mem) &&acl_pipe_is_valid_pointer( * ( (cl_mem* )arg_value), kernel) is true
  * acl_kernel.cpp:438: pipe_ptr = *arg_value
  * acl_kernel.cpp:445: 'pipe_ptr' is explicitly dereferenced.
Current status 'Analyze'

---------------------------------------------------------------------------
103 (Local) /__w/fpga-runtime-for-opencl/fpga-runtime-for-opencl/src/acl_kernel.cpp:646 NPD.CHECK.MIGHT (1:Critical) Analyze
Pointer 'sampler' checked for NULL at line 391 may be dereferenced at line 646.
  * acl_kernel.cpp:391: arg_value is true
  * acl_kernel.cpp:391: '*arg_value' is checked for NULL.
  * acl_kernel.cpp:391: * (cl_mem* )arg_value is false
  * acl_kernel.cpp:398: arg_value is true
  * acl_kernel.cpp:398: * (cl_mem* )arg_value is false
  * acl_kernel.cpp:398: arg_value&& ( * (cl_mem* )arg_value) &&arg_info->type_qualifier==ACL_ARG_TYPE_PIPE&& ( * (cl_mem* )arg_value) ->mem_object_type==4343 is false
  * acl_kernel.cpp:405: arg_value is true
  * acl_kernel.cpp:405: * (cl_mem* )arg_value is false
  * acl_kernel.cpp:566: arg_info->addr_space==ACL_ARG_ADDR_CONSTANT is true
  * acl_kernel.cpp:565: arg_info->addr_space==ACL_ARG_ADDR_GLOBAL||arg_info->addr_space==ACL_ARG_ADDR_CONSTANT is true
  * acl_kernel.cpp:573: Break statement causes loop exit.
    * acl_kernel.cpp:569: See loop head here.
  * acl_kernel.cpp:580: Break statement causes loop exit.
    * acl_kernel.cpp:576: See loop head here.
  * acl_kernel.cpp:598: arg_value is true
  * acl_kernel.cpp:599: * (cl_mem* )arg_value is false
  * acl_kernel.cpp:598: arg_value&& ( * (cl_mem* )arg_value) is false
  * acl_kernel.cpp:641: sampler = *arg_value
  * acl_kernel.cpp:646: 'sampler' is explicitly dereferenced.
Current status 'Analyze'

---------------------------------------------------------------------------
104 (Local) /__w/fpga-runtime-for-opencl/fpga-runtime-for-opencl/src/acl_kernel.cpp:646 NPD.CHECK.MIGHT (1:Critical) Analyze
Pointer 'sampler' checked for NULL at line 398 may be dereferenced at line 646.
  * acl_kernel.cpp:398: arg_value is true
  * acl_kernel.cpp:398: '*arg_value' is checked for NULL.
  * acl_kernel.cpp:398: * (cl_mem* )arg_value is false
  * acl_kernel.cpp:398: arg_value&& ( * (cl_mem* )arg_value) &&arg_info->type_qualifier==ACL_ARG_TYPE_PIPE&& ( * (cl_mem* )arg_value) ->mem_object_type==4343 is false
  * acl_kernel.cpp:405: arg_value is true
  * acl_kernel.cpp:405: * (cl_mem* )arg_value is false
  * acl_kernel.cpp:566: arg_info->addr_space==ACL_ARG_ADDR_CONSTANT is true
  * acl_kernel.cpp:565: arg_info->addr_space==ACL_ARG_ADDR_GLOBAL||arg_info->addr_space==ACL_ARG_ADDR_CONSTANT is true
  * acl_kernel.cpp:573: Break statement causes loop exit.
    * acl_kernel.cpp:569: See loop head here.
  * acl_kernel.cpp:580: Break statement causes loop exit.
    * acl_kernel.cpp:576: See loop head here.
  * acl_kernel.cpp:598: arg_value is true
  * acl_kernel.cpp:599: * (cl_mem* )arg_value is false
  * acl_kernel.cpp:598: arg_value&& ( * (cl_mem* )arg_value) is false
  * acl_kernel.cpp:641: sampler = *arg_value
  * acl_kernel.cpp:646: 'sampler' is explicitly dereferenced.
Current status 'Analyze'

---------------------------------------------------------------------------
105 (Local) /__w/fpga-runtime-for-opencl/fpga-runtime-for-opencl/src/acl_kernel.cpp:646 NPD.CHECK.MIGHT (1:Critical) Analyze
Pointer 'sampler' checked for NULL at line 599 may be dereferenced at line 646.
  * acl_kernel.cpp:598: arg_value is true
  * acl_kernel.cpp:599: '*arg_value' is checked for NULL.
  * acl_kernel.cpp:599: * (cl_mem* )arg_value is false
  * acl_kernel.cpp:598: arg_value&& ( * (cl_mem* )arg_value) is false
  * acl_kernel.cpp:641: sampler = *arg_value
  * acl_kernel.cpp:646: 'sampler' is explicitly dereferenced.
Current status 'Analyze'

---------------------------------------------------------------------------
106 (Local) /__w/fpga-runtime-for-opencl/fpga-runtime-for-opencl/src/acl_kernel.cpp:646 NPD.CHECK.MIGHT (1:Critical) Analyze
Pointer 'sampler' checked for NULL at line 405 may be dereferenced at line 646.
  * acl_kernel.cpp:405: arg_value is true
  * acl_kernel.cpp:405: '*arg_value' is checked for NULL.
  * acl_kernel.cpp:405: * (cl_mem* )arg_value is false
  * acl_kernel.cpp:566: arg_info->addr_space==ACL_ARG_ADDR_CONSTANT is true
  * acl_kernel.cpp:565: arg_info->addr_space==ACL_ARG_ADDR_GLOBAL||arg_info->addr_space==ACL_ARG_ADDR_CONSTANT is true
  * acl_kernel.cpp:573: Break statement causes loop exit.
    * acl_kernel.cpp:569: See loop head here.
  * acl_kernel.cpp:580: Break statement causes loop exit.
    * acl_kernel.cpp:576: See loop head here.
  * acl_kernel.cpp:598: arg_value is true
  * acl_kernel.cpp:599: * (cl_mem* )arg_value is false
  * acl_kernel.cpp:598: arg_value&& ( * (cl_mem* )arg_value) is false
  * acl_kernel.cpp:641: sampler = *arg_value
  * acl_kernel.cpp:646: 'sampler' is explicitly dereferenced.
Current status 'Analyze'

Summary: 10 Local
10 Total Issue(s)

pcolberg

Perfect, thanks @haoxian2 for the investigation.

pcolberg · 2022-11-23T16:01:29Z

@zibaiwan This requires your approval to be merged.

zibaiwan

Thanks @haoxian2 , @pcolberg !

haoxian2 requested a review from pcolberg November 17, 2022 03:07

zibaiwan self-requested a review November 17, 2022 18:08

zibaiwan requested changes Nov 17, 2022

View reviewed changes

haoxian2 force-pushed the fix-acl-kernel-klocwork branch from ef7a5a3 to 2cb96d2 Compare November 17, 2022 19:09

haoxian2 requested a review from zibaiwan November 17, 2022 19:09

pcolberg suggested changes Nov 17, 2022

View reviewed changes

src/acl_kernel.cpp Outdated Show resolved Hide resolved

src/acl_kernel.cpp Outdated Show resolved Hide resolved

src/acl_kernel.cpp Outdated Show resolved Hide resolved

haoxian2 force-pushed the fix-acl-kernel-klocwork branch from 2cb96d2 to 30ec62f Compare November 17, 2022 21:06

haoxian2 requested a review from pcolberg November 17, 2022 21:08

pcolberg suggested changes Nov 18, 2022

View reviewed changes

src/acl_kernel.cpp Outdated Show resolved Hide resolved

src/acl_kernel.cpp Outdated Show resolved Hide resolved

src/acl_kernel.cpp Outdated Show resolved Hide resolved

src/acl_kernel.cpp Outdated Show resolved Hide resolved

haoxian2 added 3 commits November 18, 2022 08:05

Fixed deferencing nullptr arg_value in src/acl_kernel

1501b79

Add assert for null check for pipe_ptr since acl_pipe_is_valid_pointe…

a89e3ea

…r ensures that arg_value is not Null

Add assert check for null pointer on sampler variable since acl_sampl…

6d2b722

…er_is_valid_ptr ensures pointer is not Null

haoxian2 force-pushed the fix-acl-kernel-klocwork branch from 30ec62f to 6d2b722 Compare November 18, 2022 16:13

Added extra check in unit test for NULL ARG as non-memory type

a38e1da

haoxian2 requested a review from pcolberg November 18, 2022 17:28

pcolberg assigned haoxian2 Nov 18, 2022

pcolberg added the bug Something isn't working label Nov 18, 2022

pcolberg added this to the 2023.1 milestone Nov 18, 2022

pcolberg approved these changes Nov 21, 2022

View reviewed changes

zibaiwan approved these changes Nov 23, 2022

View reviewed changes

zibaiwan merged commit 59154a3 into intel:main Nov 23, 2022

haoxian2 deleted the fix-acl-kernel-klocwork branch November 23, 2022 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Klocwork check null before dereference in src/acl_kernel.cpp #202

Klocwork check null before dereference in src/acl_kernel.cpp #202

Uh oh!

haoxian2 commented Nov 17, 2022 •

edited

Loading

Uh oh!

zibaiwan left a comment •

edited

Loading

Uh oh!

haoxian2 commented Nov 17, 2022

Uh oh!

pcolberg left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pcolberg left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pcolberg commented Nov 18, 2022

Uh oh!

haoxian2 commented Nov 21, 2022 •

edited

Loading

Uh oh!

pcolberg commented Nov 21, 2022

Uh oh!

pcolberg left a comment

Uh oh!

pcolberg commented Nov 23, 2022

Uh oh!

zibaiwan left a comment

Uh oh!

Uh oh!

Klocwork check null before dereference in src/acl_kernel.cpp #202

Klocwork check null before dereference in src/acl_kernel.cpp #202

Uh oh!

Conversation

haoxian2 commented Nov 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zibaiwan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haoxian2 commented Nov 17, 2022

Uh oh!

pcolberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pcolberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pcolberg commented Nov 18, 2022

Uh oh!

haoxian2 commented Nov 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pcolberg commented Nov 21, 2022

Uh oh!

pcolberg left a comment

Choose a reason for hiding this comment

Uh oh!

pcolberg commented Nov 23, 2022

Uh oh!

zibaiwan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

haoxian2 commented Nov 17, 2022 •

edited

Loading

zibaiwan left a comment •

edited

Loading

haoxian2 commented Nov 21, 2022 •

edited

Loading