Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix double free on Crystal::Loader#close_all #11662

Merged

Conversation

HertzDevil
Copy link
Contributor

@HertzDevil HertzDevil commented Dec 28, 2021

Crystal::Loader#finalize calls #close_all, but the FFI and loader specs call the latter manually, which means that upon a GC cycle all the dynamic library handles are closed once again. This PR prevents this by clearing the list of handles once they are closed.

Resolves #11561. I have also confirmed that #11343 with this patch will turn the LLVM 13 CI green. I managed to reproduce this locally with just:

$ make clean crystal
$ bin/crystal build -o specs spec/compiler/ffi/ffi_spec.cr spec/compiler/loader/unix_spec.cr spec/compiler/semantic/abstract_def_spec.cr
$ ./specs
#Using compiled compiler at .build/crystal
#........................Invalid memory access (signal 11) at address 0x8000
#[0x558b4b517f76] *Exception::CallStack::print_backtrace:Nil +118 in ./specs
#[0x558b4b4f4686] ~procProc(Int32, Pointer(LibC::SiginfoT), Pointer(Void), Nil) +310 in ./specs
#[0x7f84a015e200] ?? +140207598199296 in /lib/x86_64-linux-gnu/libpthread.so.0
#[0x7f84a68daee6] ?? +140207706713830 in /lib64/ld-linux-x86-64.so.2
#[0x7f84a68cdae1] _dl_exception_create +49 in /lib64/ld-linux-x86-64.so.2
#[0x7f84a002f894] _dl_signal_error +52 in /lib/x86_64-linux-gnu/libc.so.6
#[0x7f84a68ccd71] ?? +140207706656113 in /lib64/ld-linux-x86-64.so.2
#[0x7f84a002f940] _dl_catch_exception +128 in /lib/x86_64-linux-gnu/libc.so.6
#[0x7f84a002f9ff] _dl_catch_error +47 in /lib/x86_64-linux-gnu/libc.so.6
#[0x7f84a00eda59] ?? +140207597738585 in /lib/x86_64-linux-gnu/libdl.so.2
#[0x7f84a00ed364] dlclose +36 in /lib/x86_64-linux-gnu/libdl.so.2
#[0x558b4c764742] *Crystal::Loader#close_all:Nil +82 in ./specs
#[0x558b4c7646e6] *Crystal::Loader#finalize:Nil +6 in ./specs
#[0x558b4b4f7606] ~proc15Proc(Pointer(Void), Pointer(Void), Nil) +6 in ./specs
#[0x558b4c7868ae] GC_invoke_finalizers +158 in ./specs
#[0x558b4c7869fe] GC_notify_or_invoke_finalizers +174 in ./specs
#[0x558b4c787e41] GC_generic_malloc +33 in ./specs
#[0x558b4c7880d1] GC_malloc_kind_global +225 in ./specs
#[0x558b4b5cc016] *GC::malloc<UInt64>:Pointer(Void) +6 in ./specs
#[0x558b4b4f4966] ~procProc(UInt64, Pointer(Void)) +6 in ./specs
#[0x7f84a02fcb6d] ?? +140207599897453 in /lib/x86_64-linux-gnu/libpcre.so.3
#[0x7f84a0302962] pcre_study +738 in /lib/x86_64-linux-gnu/libpcre.so.3
#[0x558b4b693dae] *Regex#initialize<String, Regex::Options>:Int32 +302 in ./specs
#[0x558b4b693c6c] *Regex::new<String, Regex::Options>:Regex +92 in ./specs
#[0x558b4b4ecd9f] ~$Regex:2:init +15 in ./specs
#[0x558b4c772a77] *Crystal::OnceState#once<Pointer(Bool), Pointer(Void)>:(Pointer(Bool) | Nil) +183 in ./specs
#[0x558b4b4d3eb5] __crystal_once +37 in ./specs
#[0x558b4b4ecd7e] ~$Regex:2:read +30 in ./specs
#[0x558b4c2aa7fa] *LLVM::default_target_triple:String +106 in ./specs
#[0x558b4c2aa43a] *Crystal::Config::host_target:Crystal::Codegen::Target +42 in ./specs
#[0x558b4b70fb77] *Crystal::Program::new:Crystal::Program +423 in ./specs
#[0x558b4c7725b9] */home/quinton/crystal/crystal/spec/spec_helper.cr::new_program:Crystal::Program +9 in ./specs
#Segmentation fault

However it seems to happen even on earlier LLVM versions; I tried 9 and 11, and both of them produced the same error. I believe calling dlclose more than dlopen on the same handle is undefined behaviour and it is sheer coincidence the other CI jobs didn't break. The following will show that a double free has indeed occurred:

class Crystal::Loader
  def close_all : Nil
    @handles.each do |handle|
      unless LibC.dlclose(handle) == 0
        STDERR.puts String.new(LibC.dlerror)
      end
    end
  end
end

The same spec binary above will print shared object not open a few times. The count varies between runs of the binary for some reason.

@HertzDevil HertzDevil added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:runtime labels Dec 28, 2021
@HertzDevil
Copy link
Contributor Author

A more robust solution would be maintaining our own reference counts for each successful dlopen, to ensure the handles are correctly closed even if they were opened multiple times.

@asterite
Copy link
Member

Also as a rule of thumb, if a class defines a finalize method and another public method that ends up doing the same thing, you should use a "closed" flag to prevent double finalize.

@straight-shoota straight-shoota added this to the 1.3.0 milestone Dec 30, 2021
@straight-shoota straight-shoota merged commit 9c2d38e into crystal-lang:master Jan 3, 2022
@HertzDevil HertzDevil deleted the bug/loader-close-all branch January 4, 2022 01:54
rdp pushed a commit to rdp/crystal that referenced this pull request Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A compiler built with LLVM 13 doesn't pass the specs
4 participants