Project

General

Profile

Actions

Bug #15116

closed

Fixing issues detected by an Analysis tool.

Added by jaruga (Jun Aruga) over 5 years ago. Updated over 5 years ago.

Status:
Closed
Assignee:
-
Target version:
-
[ruby-core:88994]

Description

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/


Files

issues_report.txt (7.74 KB) issues_report.txt jaruga (Jun Aruga), 09/13/2018 01:42 PM

Updated by jaruga (Jun Aruga) over 5 years ago

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.

Updated by nobu (Nobuyoshi Nakada) over 5 years ago

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.

Actions #3

Updated by nobu (Nobuyoshi Nakada) over 5 years ago

  • Status changed from Open to Closed

Updated by jaruga (Jun Aruga) over 5 years ago

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?

Updated by nobu (Nobuyoshi Nakada) over 5 years ago

Negative fd means open(2) failed.

Updated by nobu (Nobuyoshi Nakada) over 5 years ago

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.

Updated by jaruga (Jun Aruga) over 5 years ago

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?

Updated by nobu (Nobuyoshi Nakada) over 5 years ago

Thank you, fixed it now.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0