Project

General

Profile

Bug #15116

Fixing issues detected by an Analysis tool.

Added by jaruga (Jun Aruga) 10 months ago. Updated 10 months ago.

Status:
Closed
Priority:
Normal
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

Associated revisions

Revision 64750
Added by nobu (Nobuyoshi Nakada) 10 months ago

Fix issues detected by code analysis tool (mainly Coverity).

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

[Fix GH-1956]

From: Jun Aruga jaruga@redhat.com

Revision eddd6300
Added by nobu (Nobuyoshi Nakada) 10 months ago

pty.c: more difensive

  • ext/pty/pty.c (chfunc): should not close the slave fd if it is 0..2. [ruby-core:89043] [Bug #15116]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@64766 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 64766
Added by nobu (Nobuyoshi Nakada) 10 months ago

pty.c: more difensive

  • ext/pty/pty.c (chfunc): should not close the slave fd if it is 0..2. [ruby-core:89043] [Bug #15116]

Revision 64766
Added by nobu (Nobuyoshi Nakada) 10 months ago

pty.c: more difensive

  • ext/pty/pty.c (chfunc): should not close the slave fd if it is 0..2. [ruby-core:89043] [Bug #15116]

Revision 742df62e
Added by nobu (Nobuyoshi Nakada) 10 months ago

pty.c: typo

  • ext/pty/pty.c (chfunc): fix a typo of an operator. pointed out by jaruga (Jun Aruga) at [ruby-core:89058]. [Bug #15116]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@64771 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 64771
Added by nobu (Nobuyoshi Nakada) 10 months ago

pty.c: typo

  • ext/pty/pty.c (chfunc): fix a typo of an operator. pointed out by jaruga (Jun Aruga) at [ruby-core:89058]. [Bug #15116]

Revision 64771
Added by nobu (Nobuyoshi Nakada) 10 months ago

pty.c: typo

  • ext/pty/pty.c (chfunc): fix a typo of an operator. pointed out by jaruga (Jun Aruga) at [ruby-core:89058]. [Bug #15116]

History

Updated by jaruga (Jun Aruga) 10 months 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) 10 months 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.

#3

Updated by nobu (Nobuyoshi Nakada) 10 months ago

  • Status changed from Open to Closed

Updated by jaruga (Jun Aruga) 10 months 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) 10 months ago

Negative fd means open(2) failed.

Updated by nobu (Nobuyoshi Nakada) 10 months 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) 10 months 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) 10 months ago

Thank you, fixed it now.

Also available in: Atom PDF