Bug 1714851

Summary:	issues with 'list.h' elements in clang-scan
Product:	[Community] GlusterFS	Reporter:	Amar Tumballi <atumball>
Component:	core	Assignee:	bugs <bugs>
Status:	CLOSED UPSTREAM	QA Contact:
Severity:	low	Docs Contact:
Priority:	low
Version:	mainline	CC:	bugs, jahernan, pasik
Target Milestone:	---	Keywords:	Tracking
Target Release:	---
Hardware:	Unspecified
OS:	Unspecified
Whiteboard:
Fixed In Version:		Doc Type:	If docs needed, set a value
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2020-03-12 12:54:47 UTC	Type:	Bug
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	---
oVirt Team:	---	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	---	Target Upstream Version:
Embargoed:

Description Amar Tumballi 2019-05-29 04:03:01 UTC

Description of problem:

Visit : https://build.gluster.org/job/clang-scan (and https://build.gluster.org/job/clang-scan/lastCompletedBuild/clangScanBuildBugs/ if the link still has the bugs listed). There are quite a few of issues listed, which seems to be false-positive, and are 'list' related.

Shyam's points:

Posting an older context from when I was looking at certain list related clang scan issues (see below).

There is a class of clang scan related issues where using certain list macros that are not empty list safe is present in the code. I am going to assume this maybe one of those cases, if not please ignore this comment.

<snip>

Clang bug type: Result of operation is garbage or undefined
Where: glusterfs/api/src/glfs-fops.c	glfs_recall_lease_fd	5208
Other instances (there are more but not analyzed)
glusterfs/xlators/features/locks/src/clear.c	clrlk_clear_inodelk	257
glusterfs/xlators/features/locks/src/clear.c	clrlk_clear_entrylk	360
glusterfs/xlators/features/locks/src/entrylk.c	pl_entrylk_client_cleanup
1075
glusterfs/xlators/features/locks/src/inodelk.c	pl_inodelk_client_cleanup	694

Reason: list_for_each_entry_safe is not "Empty list" safe

The places where this is caught is when we use uninitialized stack
variables (for the most part), but in reality the pattern is incorrect
and needs correction.

We need to take a few actions here:
- Review the list macro usage across the code
- Update the list.h to a more later version from the kernel sources?
  - Later kernel sources call out the unsafe nature of the same
    - list_first_entry: https://github.com/torvalds/linux/blob/cd6c84d8f0cdc911df435bb075ba22ce3c605b07/include/linux/list.h#L473-L476
      - Calls out "Note, that list is expected to be not empty"
    - list_first_entry Called from list_for_each_entry_safe: https://github.com/torvalds/linux/blob/cd6c84d8f0cdc911df435bb075ba22ce3c605b07/include/linux/list.h#L649-L654
      - Thus making it also needing to adhere to "Note, that list is expected to be not empty"

</snip>

Version-Release number of selected component (if applicable):


---
Need to think and handle it in a right way.

Comment 1 Xavi Hernandez 2019-06-03 07:57:57 UTC

I'm not sure we really have an issue in list_for_each_entry_safe(). Even if the list is empty and list_first_entry() is used (which is true that it returns a bad pointer when list is empty), what we get is a pointer to an invalid structure. That's true. However, the macro only dereferences the 'list' field, which is guaranteed to be valid, even if the list is empty, and in this case it will exit the loop, so no unsafe pointers will be passed to the body of the loop.

Additionally, clang-scan complains about the entry pointer being NULL inside the loop. The only case where this can happen is when the list is not initialized with INIT_LIST_HEAD() and the memory is cleared with 0's. However clang-scan doesn't provide a trace path from allocation to list_for_each_entry_safe() call where this can be proved. So my guess is that clang-scan assumes that any value is possible for a given pointer passed as an argument. In that case many false-positives will appear, since it's assuming something that is not true most of the cases.

Comment 2 Worker Ant 2020-03-12 12:54:47 UTC

This bug is moved to https://github.com/gluster/glusterfs/issues/964, and will be tracked there from now on. Visit GitHub issues URL for further details