Memory corruption when DeleteFile() is called for a file explicitly closed by Close()

[percona-ysorokin]

rocksdb.use_direct_io_for_flush_and_compaction MTR test case run under ASan/Valgrind helped to reveal the following heap-use-after-free problem.

==527273== Invalid read of size 8
==527273==    at 0x2C039C94: rocksdb::ZoneFile::CloseWR() (io_zenfs.cc:225)
==527273==    by 0x2C03A9D3: rocksdb::ZonedWritableFile::~ZonedWritableFile() (io_zenfs.cc:424)
==527273==    by 0x2C03AA1B: rocksdb::ZonedWritableFile::~ZonedWritableFile() (io_zenfs.cc:426)
==527273==    by 0x2B9575EB: std::default_delete<rocksdb::FSWritableFile>::operator()(rocksdb::FSWritableFile*) const (unique_ptr.h:85)
==527273==    by 0x2B9562F7: std::unique_ptr<rocksdb::FSWritableFile, std::default_delete<rocksdb::FSWritableFile> >::~unique_ptr() (unique_ptr.h:361)
==527273==    by 0x2BC7695D: rocksdb::(anonymous namespace)::CompositeWritableFileWrapper::~CompositeWritableFileWrapper() (composite_env.cc:105)
==527273==    by 0x2BC76989: rocksdb::(anonymous namespace)::CompositeWritableFileWrapper::~CompositeWritableFileWrapper() (composite_env.cc:105)
==527273==    by 0x2BB116CF: std::default_delete<rocksdb::WritableFile>::operator()(rocksdb::WritableFile*) const (unique_ptr.h:85)
==527273==    by 0x2BB1140B: std::unique_ptr<rocksdb::WritableFile, std::default_delete<rocksdb::WritableFile> >::~unique_ptr() (unique_ptr.h:361)
==527273==    by 0x2C04F0F4: myrocks::rocksdb_init_internal(void*) (ha_rocksdb.cc:5582)
==527273==    by 0x2C05336C: myrocks::rocksdb_init_func(void*) (ha_rocksdb.cc:6144)
==527273==    by 0x3A9FFF0: ha_initialize_handlerton(st_plugin_int*) (handler.cc:801)
==527273==    by 0x3FA06AA: plugin_initialize(st_plugin_int*) (sql_plugin.cc:1288)
==527273==    by 0x3FA0E36: plugin_init_initialize_and_reap() (sql_plugin.cc:1444)
==527273==    by 0x3FA1E37: plugin_register_dynamic_and_init_all(int*, char**, int) (sql_plugin.cc:1765)
==527273==    by 0x3D26FC1: init_server_components() (mysqld.cc:6374)
==527273==  Address 0x27d08738 is 40 bytes inside a block of size 136 free'd
==527273==    at 0x9C188FF: operator delete(void*, unsigned long) (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==527273==    by 0x2C039C7C: rocksdb::ZoneFile::~ZoneFile() (io_zenfs.cc:222)
==527273==    by 0x2C021A7D: rocksdb::ZenFS::DeleteFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) (fs_zenfs.cc:416)
==527273==    by 0x2C022EF7: rocksdb::ZenFS::DeleteFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, rocksdb::IOOptions const&, rocksdb::IODebugContext*) (fs_zenfs.cc:589)
==527273==    by 0x2BC77561: rocksdb::CompositeEnv::DeleteFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (composite_env_wrapper.h:90)
==527273==    by 0x2C04F0D6: myrocks::rocksdb_init_internal(void*) (ha_rocksdb.cc:5581)
==527273==    by 0x2C05336C: myrocks::rocksdb_init_func(void*) (ha_rocksdb.cc:6144)
==527273==    by 0x3A9FFF0: ha_initialize_handlerton(st_plugin_int*) (handler.cc:801)
==527273==    by 0x3FA06AA: plugin_initialize(st_plugin_int*) (sql_plugin.cc:1288)
==527273==    by 0x3FA0E36: plugin_init_initialize_and_reap() (sql_plugin.cc:1444)
==527273==    by 0x3FA1E37: plugin_register_dynamic_and_init_all(int*, char**, int) (sql_plugin.cc:1765)
==527273==    by 0x3D26FC1: init_server_components() (mysqld.cc:6374)
==527273==    by 0x3D2B567: mysqld_main(int, char**) (mysqld.cc:7525)
==527273==    by 0x3A7BC8C: main (main.cc:25)
==527273==  Block was alloc'd at
==527273==    at 0x9C15CE3: operator new(unsigned long) (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==527273==    by 0x2C022106: rocksdb::ZenFS::NewWritableFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, rocksdb::FileOptions const&, std::unique_ptr<rocksdb::FSWritableFile, std::default_delete<rocksdb::FSWritableFile> >*, rocksdb::IODebugContext*) (fs_zenfs.cc:474)
==527273==    by 0x2BC76027: rocksdb::CompositeEnv::NewWritableFile(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::unique_ptr<rocksdb::WritableFile, std::default_delete<rocksdb::WritableFile> >*, rocksdb::EnvOptions const&) (composite_env.cc:320)
==527273==    by 0x2C04F030: myrocks::rocksdb_init_internal(void*) (ha_rocksdb.cc:5577)
==527273==    by 0x2C05336C: myrocks::rocksdb_init_func(void*) (ha_rocksdb.cc:6144)
==527273==    by 0x3A9FFF0: ha_initialize_handlerton(st_plugin_int*) (handler.cc:801)
==527273==    by 0x3FA06AA: plugin_initialize(st_plugin_int*) (sql_plugin.cc:1288)
==527273==    by 0x3FA0E36: plugin_init_initialize_and_reap() (sql_plugin.cc:1444)
==527273==    by 0x3FA1E37: plugin_register_dynamic_and_init_all(int*, char**, int) (sql_plugin.cc:1765)
==527273==    by 0x3D26FC1: init_server_components() (mysqld.cc:6374)
==527273==    by 0x3D2B567: mysqld_main(int, char**) (mysqld.cc:7525)
==527273==    by 0x3A7BC8C: main (main.cc:25)

This problem can be narrowed down to the following code fragment

      std::unique_ptr<rocksdb::WritableFile> file;
      soptions.use_direct_writes = true;
      check_status = env->NewWritableFile(fname, &file, soptions);
      if (file != nullptr) {
        file->Close();
      }
      env->DeleteFile(fname);

env can be ignored for now - this is just a wrapper for ZenFS class. Here, we basically do the following.

• Create a new file via NewWritableFile() - the result is put into file local variable (std::unique_ptr)
• We close the file explicitly via Close() method
• We delete the closed file explicitly via DeleteFile() method - this in turns removes corresponding zone file from the files_ map and calls delete zoneFile
• At the end of scope std::unique_ptr's destructor is called causing a call to ZonedWritableFile's destructor which in turn calls zoneFile_->CloseWR() on an already deleted (freed) zone.

The problem can definitely be fixed from the library user side - by calling DeleteFile() when file (std::unique_ptr) is out of scope.

      {
        std::unique_ptr<rocksdb::WritableFile> file;
        soptions.use_direct_writes = true;
        check_status = env->NewWritableFile(fname, &file, soptions);
      }
      env->DeleteFile(fname);

However, the question is should the library itself take care of such scenario?

[yhr]

fix merged