Fixed coverity issues in src/acl_mem.cpp #219
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
haoxian2
force-pushed
the
coverity-acl-mem
branch
from
f723b2f to
f171b99
Compare
pcolberg
reviewed
Dec 3, 2022
haoxian2
force-pushed
the
coverity-acl-mem
branch
2 times, most recently
from
3046edd to
207bd89
Compare
haoxian2
force-pushed
the
coverity-acl-mem
branch
from
207bd89 to
5eea2ac
Compare
pcolberg
reviewed
Dec 6, 2022
haoxian2
force-pushed
the
coverity-acl-mem
branch
from
5eea2ac to
72fd012
Compare
haoxian2
force-pushed
the
coverity-acl-mem
branch
2 times, most recently
from
68cbc2a to
6708bc5
Compare
pcolberg
reviewed
Dec 8, 2022
pcolberg
reviewed
Dec 8, 2022
Changed a lot of the && to || because CL_MEM_OBJECT_IMAGE1D, CL_MEM_OBJECT_IMAGE1D_ARRAY, and CL_MEM_OBJECT_IMAGE1D_BUFFER are all defined as strictly different values respectively 0x10F4, 0x10F5, and 0x10F6. Therefore that section of the statement would never be accessed. Logically, if the object is a 1D image, no matter which one it is, the slice pitch will not have a height and would just be equal to the row pitch, therefore I think logical OR makes sense. (line 2957) ((map_flags & CL_MAP_READ) & (map_flags & CL_MAP_WRITE_INVALIDATE_REGION)), both will only return the common bit between map_flags and the 2nd value, CL_MAP_READ being 1 and CL_MAP_WRITE_INVALIDATE_REGION being 4, but there are no common bits between 1 and 4, therefore this statement will always be false. From the comments, I believe that this block takes care of the state where map_flags contains both one of CL_MAP_READ or CL_MAP_WRITE and CL_MAP_WRITE_INVALIDATE_REGION. and therefore should replace the middle & to &&.
1D image does not have initialization of height and depth members. Some functions check for that before they check for height and depth. Added an image with 0 height and 0 depth to make sure that these functions don't mistakenly use the height and depth in image_desc.
pcolberg
force-pushed
the
coverity-acl-mem
branch
from
6708bc5 to
3477aef
Compare
pcolberg
reviewed
Dec 8, 2022
haoxian2
force-pushed
the
coverity-acl-mem
branch
from
3477aef to
dbda512
Compare
(Line 2036) Previously, if image is NULL, src_element_size is set to 0. However, there are a bunch of 'image' dereference after this check, which does not make sense since this if statement makes it seem like this function can handle a NULL image. Therefore, I replaced it with return CL_INVALID_MEM_OBJECT like some of the other functions. (Line 6829) Except these two lines, all the other line querying about mem->block_allocation are contained in the block with a null check for mem->block_allocation. Moving the two lines with the other lines.
(Line 4172) Missing free for a malloc object before returning, same with (line 4183) Solution: Turning `needs_release_on_fail` into vector since it is only present in the scope of this function
pcolberg
force-pushed
the
coverity-acl-mem
branch
from
dbda512 to
491857d
Compare
pcolberg
approved these changes
Dec 10, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixed coverity issues in acl_mem.cpp: Constant expression result
This issues happen when a condition can only have one outcome.
image_heightas per specifications, it should be an||operator instead of&&operator.((map_flags & CL_MAP_READ) & (map_flags & CL_MAP_WRITE_INVALIDATE_REGION)), both will only return the common bit betweenmap_flagsand the 2nd value,CL_MAP_READbeing 1 andCL_MAP_WRITE_INVALIDATE_REGIONbeing 4, but there are no common bits between 1 and 4, therefore this statement will always be false. From the comments, I believe that this block takes care of the state wheremap_flagscontains both one ofCL_MAP_READorCL_MAP_WRITEandCL_MAP_WRITE_INVALIDATE_REGION. and therefore should replace the middle&&to&.Added new checks in acl_mem_test.cpp
Checking if
enqueuefunctions can handle 1D image. These functions access theimage_desc.heightvariable, making sure it is initialized by default.Fixed coverity issue in acl_mem.cpp: Dereference after null check
imageisNULL,src_element_sizeis set to0. However, there are a bunch of 'image' dereference after this check, which does not make sense since thisifstatement makes it seem like this function can handle aNULL image. Therefore, I replaced it withreturn CL_INVALID_MEM_OBJECTlike some of the other functions. Rewrote some blocks of code that are similar to this one to conform to better verticality of code.mem->block_allocationare contained in the block with a null check formem->block_allocation. Moving the two lines with the other lines.Fixed coverity issue in acl_mem.cpp: Memory leak
(Line 4172) Missing
freefor a malloc object before returning, same with (line 4183)Turned
needs_release_on_failinto a vector to avoidmallocand use built in destructor when it goes out of scope.