Describe the bug
The open() function returns a file descriptor on success or -1 on failure. In the given code snippet, the condition if (!fd) is used to check if open() failed. However, this condition will evaluate to true if the file descriptor is 0, which could be a valid value.
redis/src/debug.c.
To fix this, you should check if open() returned -1 specifically to determine if it failed. The correct condition would be if (fd == -1).
Comment From: enjoy-binbin
this look right, do you want to issue a fix?
Comment From: shahsb
Describe the bug
The open() function returns a file descriptor on success or -1 on failure. In the given code snippet, the condition if (!fd) is used to check if open() failed. However, this condition will evaluate to true if the file descriptor is 0, which could be a valid value.
redis/src/debug.c.
To fix this, you should check if open() returned -1 specifically to determine if it failed. The correct condition would be if (fd == -1).
This is a good catch - @Chaoshuai-Li . Could you please elaborate how did you catch this?
Comment From: Chaoshuai-Li
Describe the bug The open() function returns a file descriptor on success or -1 on failure. In the given code snippet, the condition if (!fd) is used to check if open() failed. However, this condition will evaluate to true if the file descriptor is 0, which could be a valid value. redis/src/debug.c. To fix this, you should check if open() returned -1 specifically to determine if it failed. The correct condition would be if (fd == -1).
This is a good catch - @Chaoshuai-Li . Could you please elaborate how did you catch this?
We only discovered it while using our self-developed static analysis tool to analyze Redis. There are also other resource leak vulnerabilities that we found during our audit, which seem to be triggered as well. Could you please perform an audit?
We only discovered it while using our self-developed static analysis tool to analyze Redis. There are also other resource leak vulnerabilities that we found during our audit, which seem to be triggered as well. Could you please perform an audit?
Comment From: Chaoshuai-Li
this look right, do you want to issue a fix?
Sorry for the delayed response. Your fix is correct and straightforward.
Comment From: enjoy-binbin
We only discovered it while using our self-developed static analysis tool to analyze Redis. There are also other resource leak vulnerabilities that we found during our audit, which seem to be triggered as well. Could you please perform an audit?
thanks, i scanned the parts you mentioned. Some have been fixed in unstable, and the rest are actually false positives. Can you scan for the unstable branch? i can deal with them all at once.
Comment From: Chaoshuai-Li
We only discovered it while using our self-developed static analysis tool to analyze Redis. There are also other resource leak vulnerabilities that we found during our audit, which seem to be triggered as well. Could you please perform an audit?
thanks, i scanned the parts you mentioned. Some have been fixed in unstable, and the rest are actually false positives. Can you scan for the unstable branch? i can deal with them all at once.
Is the usage of fstat() function in lines 235, 346, and 382 of the src/redis-check-aof.c file in the unstable branch correct? Please confirm if it is a false positive. The inconsistent code style in Redis regarding the usage of fstat() does not ensure that the opened file resources do not need to be released. It should follow the consistent code style as shown in src/util.c.
Comment From: Chaoshuai-Li
We only discovered it while using our self-developed static analysis tool to analyze Redis. There are also other resource leak vulnerabilities that we found during our audit, which seem to be triggered as well. Could you please perform an audit?
thanks, i scanned the parts you mentioned. Some have been fixed in unstable, and the rest are actually false positives. Can you scan for the unstable branch? i can deal with them all at once.
Is the usage of fstat() function in lines 235, 346, and 382 of the src/redis-check-aof.c file in the unstable branch correct? Please confirm if it is a false positive. The inconsistent code style in Redis regarding the usage of fstat() does not ensure that the opened file resources do not need to be released. It should follow the consistent code style as shown in src/util.c.
@enjoy-binbin Sorry to bother you. What are your opinions on these three inspection results?
Comment From: enjoy-binbin
they are false positive since we will do a exit after the check, but i do agree we should cleanup the resource.
I noticed them while working on #12958 and planned to fix the issues in redis-check-aof.c there