Project

General

Profile

Actions

Bug #18516

closed

Memory leak on aliasing method to itself

Added by ibylich (Ilya Bylich) 5 months ago. Updated 3 months ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Target version:
-
ruby -v:
ruby 3.2.0dev
[ruby-core:107287]

Description

The following code produces a memory leak:

class A
  1.upto(Float::INFINITY) do |i|
    define_method(:"foo_#{i}") {}

    alias :"foo_#{i}" :"foo_#{i}"

    remove_method :"foo_#{i}"
  end
end

It is very artificial, but it's required for LSAN/Valgrind integration.

The reason why it leaks is the following:

  • alias foo foo increments alias_count even if no alias is created
  • later during GC rb_free_method_entry is called that in turn calls rb_method_definition_release that calls free only if alias_count + complemented_count == 0.
  • In this particular case alias_count is 1, and so no cleanup happens.

I've been asked to create tickets for memory leaks that I find during LSAN integration if the leak affects previous versions of Ruby. From what I see it does.

Possible fix: https://github.com/ruby/ruby/pull/5492. But like I mentioned in PR I'm not sure if it's correct, @nobu (Nobuyoshi Nakada) mentioned in my main PR aliasing method to itself is used to swallow some warning on method redefinition, but I don't what's the case there.

Updated by ibylich (Ilya Bylich) 5 months ago

As @nobu (Nobuyoshi Nakada) mentioned the following code gives no warnings (in verbose mode):

class A
  def foo; end
  alias foo foo;
  def foo; end
end

but if we stop increment alias_count on alias foo foo the warning appears (because alias foo foo becomes a real noop, with absolutely no side effect).

Updated by ufuk (Ufuk Kayserilioglu) 5 months ago

ibylich (Ilya Bylich) wrote in #note-1:

but if we stop increment alias_count on alias foo foo the warning appears (because alias foo foo becomes a real noop, with absolutely no side effect).

Rails heavily depends on this behaviour and if this is changed it will end up raising a lot of warnings. I am not even sure if there is another way to replace methods without raising any warnings, if this is taken away.

Actions #4

Updated by nobu (Nobuyoshi Nakada) 5 months ago

  • Status changed from Open to Closed

Applied in changeset git|e89d80702bd98a8276243a7fcaa2a158b3bfb659.


Fix memory leak at the same named alias [Bug #18516]

When aliasing a method to the same name method, set a separate bit
flag on that method definition, instead of the reference count
increment. Although this kind of alias has no actual effect at
runtime, is used as the hack to suppress the method re-definition
warning.

Updated by byroot (Jean Boussier) 5 months ago

  • Backport changed from 2.6: UNKNOWN, 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN to 2.6: WONTFIX, 2.7: REQUIRED, 3.0: REQUIRED, 3.1: REQUIRED

I took the liberty to set the backport field.

I was able to repro on 2.6, 2.7,3.0 and 3.1. However 2.6 is security only, I don't think it qualifies.

Updated by naruse (Yui NARUSE) 5 months ago

  • Backport changed from 2.6: WONTFIX, 2.7: REQUIRED, 3.0: REQUIRED, 3.1: REQUIRED to 2.6: WONTFIX, 2.7: REQUIRED, 3.0: REQUIRED, 3.1: DONE

ruby_3_1 42c9ef769f210d88241a114395dd5ffc27b2fb87 merged revision(s) 7ff1bf317887c0d7b21e91ad548d07b9f05c540c,e89d80702bd98a8276243a7fcaa2a158b3bfb659.

Updated by nagachika (Tomoyuki Chikanaga) 4 months ago

  • Backport changed from 2.6: WONTFIX, 2.7: REQUIRED, 3.0: REQUIRED, 3.1: DONE to 2.6: WONTFIX, 2.7: REQUIRED, 3.0: DONE, 3.1: DONE

ruby_3_0 6175823bab28b5d12f66371d67d006df37751fbc merged revision(s) 7ff1bf317887c0d7b21e91ad548d07b9f05c540c,e89d80702bd98a8276243a7fcaa2a158b3bfb659.

Actions #8

Updated by usa (Usaku NAKAMURA) 3 months ago

  • Backport changed from 2.6: WONTFIX, 2.7: REQUIRED, 3.0: DONE, 3.1: DONE to 2.6: WONTFIX, 2.7: DONE, 3.0: DONE, 3.1: DONE
Actions

Also available in: Atom PDF