Bug #15116: Fixing issues detected by an Analysis tool.

• Status: Closed
• Assignee: -
• Target version: -
• Backport: 2.3: UNKNOWN, 2.4: UNKNOWN, 2.5: UNKNOWN

[jaruga (Jun Aruga)]

When running a code analysis tool including several sub tools (mainly Coverty [1]), some issues were detected by it.
You can refer attached issues_report.txt for detail.

Some issues in the issues_report.txt might be false positive.
(Those might be wrongly detected.)

I tried to fix those by below 2 pull-requests.
https://github.com/ruby/ruby/pull/1956
https://github.com/ruby/net-telnet/pull/15

The summary is

• Fix leaked storage in addr2line.c.
• Fix passing freed pointer as an argument in gc.c.
• Fix leaked handle variable "n" in process.c.
• Fix for "top_root" leaking the resource.

After above patches, the issues were not detected.
But I need your help to check if my code is valid.

Thank you.

[1] https://scan.coverity.com/

[jaruga (Jun Aruga)]

To be clear, those 9 issues about mainly leaked resource in the attached report file are marked as "important" by the analysis tool for ruby 2.5.1 + some patch files.

Total 9

• 1 COPY_PASTE_ERROR
• 7 RESOURCE_LEAK
• 1 USE_AFTER_FREE

However as a "not important" issues, around 1000 issues were detected by the tool for the ruby 2.5.1.
I am considering how to deal with this or report those.
I might open an another ticket for that.

[nobu (Nobuyoshi Nakada)]

Thank you for the analyzing.

jaruga (Jun Aruga) wrote:

The summary is

• Fix leaked storage in addr2line.c.
• Fix for "top_root" leaking the resource.

The above seems valid.

And the followings are false positive.

• Fix passing freed pointer as an argument in gc.c.

The pointer is never dereferenced but just the pointer value is shown.

• Fix leaked handle variable "n" in process.c.

If n is 0..2, dup2 to the same fd does nothing, and n must not be closed.

[nobu (Nobuyoshi Nakada)]

• Status changed from Open to Closed

[jaruga (Jun Aruga)]

Thank you for checking my code!

Fix leaked handle variable "n" in process.c.

If n is 0..2, dup2 to the same fd does nothing, and n must not be closed.

About above thing,

I compared process.c#rb_daemon with pty.c#chfunc .
Similar implementation, but a little different.

https://github.com/ruby/ruby/blob/trunk/ext/pty/pty.c#L130-L146

In the case of pty.c, the "leaked handle" was not detected for the return value of rb_cloexec_open. even when close function can be executed for for the return value "slave" (= file descriptor) == 0, 1 or 2.

In process.c, when the return value n < 0, is it no problem?

[nobu (Nobuyoshi Nakada)]

Negative fd means open(2) failed.

[nobu (Nobuyoshi Nakada)]

POSIX states successfully opened file descriptors should be non-negative.

http://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html#tag_16_357_04

RETURN VALUE

Upon successful completion, these functions shall open the file and return a non-negative integer representing the file descriptor. Otherwise, these functions shall return -1 and set errno to indicate the error. If -1 is returned, no files shall be created or modified.

pty.c should be more defensive too, thank you for pointing out.

[jaruga (Jun Aruga)]

Thank you for clarifying it!

By the way, I saw your commit for the fix.

https://github.com/ruby/ruby/commit/eddd630

-    close(slave);
+    if (slave < 0 && slave > 2) (void)!close(slave);

It seems that the close is never executed.
That is if (slave < 0 || slave > 2) (void)!close(slave);, isn't it?

[nobu (Nobuyoshi Nakada)]

Thank you, fixed it now.