Fix origin iclass pointer for modules

[jeremyevans]

If a module has an origin, and that module is included in another
module or class, previously the iclass created for the module had
an origin pointer to the module's origin instead of the iclass's
origin.

Setting the origin pointer correctly requires using a stack, since
the origin iclass is not created until after the iclass itself.
Implement the stack to track origin pointers and assign them
correctly.

Correctly assigning the origin pointers in the iclass caused a
use-after-free in GC. Fix this by not freeing the method table
for origin iclasses if it is iclass for a module. I think this
is correct because the method table would be owned by the
module itself in that case, but I'm not sure. It is possible
this leads to memory leak, but someone with a better
understanding of the garbage collector should probably determine
that.

This also includes a fix for Module#included_modules to skip
iclasses with origins.

Fixes [Bug #16736]

[nobu]

class.c:935:22: warning: unused variable 'orig_iclass' [-Wunused-variable]

[jeremyevans]

Thanks, I'll fix this.

[nobu]

Can't this cause potential memory leak, when

[jeremyevans]

This pointer is freed later when the origin iclass is created. However, if there is an exception raised between when this pointer is created and when it is freed, then that would cause a small memory leak (12-24 bytes depending on arch * number of items in stack). I do not know if any of the functions in thewhile loop can raise an exception, but it's complex enough that I cannot rule it out. Should I switch this to use rb_alloc_tmp_buffer2?

[jeremyevans]

I figured out a better approach, by storing the information in a hidden Ruby array. This should only increase memory by 8-16 bytes per module origin iclass. Assuming it passes CI, I'll commit it.

[nobu]

In this loop, rb_include_class_new, rb_module_add_to_subclasses_list, rb_module_add_to_subclasses_list and add_refined_method_entry_i can cause NoMemoryError.
How about pushing iclass and origin to a hidden array?

[jeremyevans]

Already implemented in the latest commit. :)

[nobu]

I mean pushing to a single array instead of chaining.

        add_subclass = TRUE;
        if (module != RCLASS_ORIGIN(module)) {
            if (!origin_stack) origin_stack = rb_ary_tmp_new(2);
            VALUE origin[2] = {iclass, RCLASS_ORIGIN(module)};
            rb_ary_cat(2, origin, origin_stack);
        }
        else if (origin_stack && (origin_len = RARRAY_LEN(ary)) > 1 &&
                 RARRAY_AREF(origin_stack, origin_len - 1) == module) {
            RCLASS_SET_ORIGIN(RARRAY_AREF(origin_stack, (origin_len -= 2)), iclass);
            rb_ary_resize(origin_stack, origin_len);
            add_subclass = FALSE;
        }

[jeremyevans]

Sure, I'll push another commit that uses that approach.

[nobu]

Seems unnecessary change.

[jeremyevans]

Correct, I'll fix this.

[jeremyevans]

The AppVeyor failures seem unrelated due to merge issues with Git. However, the MinGW failure may be an actual issue (ruby crashes during tests, and at different places in different runs). Unfortuntately, the MinGW failure doesn't provide debugging output, so I'm not sure where to start in regards to determining whether this is an actual issue. Can someone with access to MinGW try this PR and see if it does crash, and if so, provide a backtrace?

[jeremyevans]

With the freeing of the iclass method tables being removed, this had a clean CI run. However, I assume this leaks memory and is therefore not acceptable. This is a strong indication there is a double free or use after free on the method table, indicating that we need to change the condition on when to free the method table. However, I'm not sure what the condition should be changed to. I originally thought this issue was specific to Windows, but this CI run also shows a crash on Ubuntu, so maybe it is just more common on Windows. I can't seem to recreate the problem locally on OpenBSD, even with all memory checking turned on.

Previously, iclass method tables were freed only for iclasses that were origins. This changes it to only free iclass method tables if the iclass also represents a class and not a module. Leaving the code as it was previously (always freeing origin iclasses even for module iclasses) would always crash during testing.

[jeremyevans]

I've found the bug that was previously causing this to fail. For iclass origins (created when a class/module includes a module with an origin), the method table is shared with the module origin and shouldn't be garbage collected separately. I added a flag to the iclass origin and had the garbage collector skip marking/freeing/moving the method table if the flag is set. This fixes the issue and I checked that it does not introduce a memory leak.