root icon indicating copy to clipboard operation
root copied to clipboard

[core] kNotDeleted mechanism is broken on some platforms

Open eguiraud opened this issue 3 years ago • 0 comments

Full discussion at https://github.com/root-project/roottest/pull/880 .

Taking a look with gdb it looks like kNotDeleted is reset, but then those bits are immediately modified again by _int_free:

#0  tcache_put (tc_idx=0, chunk=0x55555562e400) at malloc.c:3183
#1  _int_free (av=0x7ffff4dfdbc0 <main_arena>, p=0x55555562e400, have_lock=0) at malloc.c:4481
#2  0x00007ffff4c9c8f3 in __GI___libc_free (mem=<optimized out>) at malloc.c:3391
#3  0x00007ffff7b30e64 in TStorage::ObjectDealloc (vp=0x55555562e410) at ../core/base/src/TStorage.cxx:362
#4  0x00007ffff7b14518 in TObject::operator delete (ptr=0x55555562e410) at ../core/base/src/TObject.cxx:1001
#5  0x00007ffff7b11cba in TObject::~TObject (this=0x55555562e410, __in_chrg=<optimized out>) at ../core/base/src/TObject.cxx:91
#6  0x0000555555556242 in main () at foo.cpp:9

where the line that accidentally sets the bit again is 3181 here:

 3172  /* Caller must ensure that we know tc_idx is valid and there's room
 3173     for more chunks.  */
 3174  static __always_inline void
 3175  tcache_put (mchunkptr chunk, size_t tc_idx)
 3176  {
 3177    tcache_entry *e = (tcache_entry *) chunk2mem (chunk);
 3178
 3179    /* Mark this chunk as "in the tcache" so the test in _int_free will
 3180       detect a double free.  */
 3181    e->key = tcache_key;
 3182
 3183    e->next = PROTECT_PTR (&e->next, tcache->entries[tc_idx]);

and this version of the test circumvents that behavior of _int_free so it does not crash (at least on my laptop):

  void *mem = malloc(sizeof(TObject));
  auto o = new (mem) TObject();
  auto l = new TList();
  l->SetName("my own list");
  l->Add(o);
  o->~TObject();
  l->Clear();
  free(mem);

If my understanding is correct, this also means that the test failure is real in the sense that the kNotDeleted mechanism does not correctly work on platforms where free has that behavior.

From Philippe (https://github.com/root-project/roottest/pull/880#issuecomment-1239663370):

We (I) need to extend the core library to detect when there is a memory checker (or similar) that leads to the memory being salted after a delete and in that case disable our own already-deleted test (since it is now ineffective anyway and assumingly it is already doing the job of warning the user about use-after-delete). And subsequently the test needs to also be disabled in those cases.

eguiraud avatar Sep 08 '22 08:09 eguiraud