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

[core] kNotDeleted mechanism is broken on some platforms #11330

Closed
eguiraud opened this issue Sep 8, 2022 · 0 comments · Fixed by #11334
Closed

[core] kNotDeleted mechanism is broken on some platforms #11330

eguiraud opened this issue Sep 8, 2022 · 0 comments · Fixed by #11334
Assignees
Milestone

Comments

@eguiraud
Copy link
Member

eguiraud commented Sep 8, 2022

Full discussion at root-project/roottest#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 (root-project/roottest#880 (comment)):

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.

pcanal added a commit to pcanal/root that referenced this issue Sep 8, 2022
On some platform operator delete taints the memory, so even right after the deletion the information
stored by ~TObject is already erased.  On those platform we no longer rely on the kNotDelete bit hack
and rely on the system (which has tainted the memory assumingly for a reason) to detect the
use-after-delete problems.

Introduce 2 new functions.

TObject::IsDestructed (used by TClonesArray) that detects that the destructor has been run and is
active in all configuration.  This should be used if the code knows that the memory has not been
freed/deleted.

ROOT::Detail::HasBeenDeleted(TObject*) with returns true if the platform does not taint the memory
and if the kNotDeleted is not set (in all other case, it returns false)

This fixes root-project#11330
pcanal added a commit to pcanal/root that referenced this issue Sep 8, 2022
On some platform operator delete taints the memory, so even right after the deletion the information
stored by ~TObject is already erased.  On those platform we no longer rely on the kNotDelete bit hack
and rely on the system (which has tainted the memory assumingly for a reason) to detect the
use-after-delete problems.

Introduce 2 new functions.

TObject::IsDestructed (used by TClonesArray) that detects that the destructor has been run and is
active in all configuration.  This should be used if the code knows that the memory has not been
freed/deleted.

ROOT::Detail::HasBeenDeleted(TObject*) with returns true if the platform does not taint the memory
and if the kNotDeleted is not set (in all other case, it returns false)

This fixes root-project#11330
pcanal added a commit to pcanal/root that referenced this issue Sep 9, 2022
On some platform operator delete taints the memory, so even right after the deletion the information
stored by ~TObject is already erased.  On those platform we no longer rely on the kNotDelete bit hack
and rely on the system (which has tainted the memory assumingly for a reason) to detect the
use-after-delete problems.

Introduce 2 new functions.

TObject::IsDestructed (used by TClonesArray) that detects that the destructor has been run and is
active in all configuration.  This should be used if the code knows that the memory has not been
freed/deleted.

ROOT::Detail::HasBeenDeleted(TObject*) with returns true if the platform does not taint the memory
and if the kNotDeleted is not set (in all other case, it returns false)

This fixes root-project#11330
pcanal added a commit to pcanal/root that referenced this issue Sep 9, 2022
On some platform operator delete taints the memory, so even right after the deletion the information
stored by ~TObject is already erased.  On those platform we no longer rely on the kNotDelete bit hack
and rely on the system (which has tainted the memory assumingly for a reason) to detect the
use-after-delete problems.

Introduce 2 new functions.

TObject::IsDestructed (used by TClonesArray) that detects that the destructor has been run and is
active in all configuration.  This should be used if the code knows that the memory has not been
freed/deleted.

ROOT::Detail::HasBeenDeleted(TObject*) with returns true if the platform does not taint the memory
and if the kNotDeleted is not set (in all other case, it returns false)

This fixes root-project#11330
pcanal added a commit to pcanal/root that referenced this issue Sep 9, 2022
On some platform operator delete taints the memory, so even right after the deletion the information
stored by ~TObject is already erased.  On those platform we no longer rely on the kNotDelete bit hack
and rely on the system (which has tainted the memory assumingly for a reason) to detect the
use-after-delete problems.

Introduce 2 new functions.

TObject::IsDestructed (used by TClonesArray) that detects that the destructor has been run and is
active in all configuration.  This should be used if the code knows that the memory has not been
freed/deleted.

ROOT::Detail::HasBeenDeleted(TObject*) with returns true if the platform does not taint the memory
and if the kNotDeleted is not set (in all other case, it returns false)

This fixes root-project#11330
pcanal added a commit to pcanal/root that referenced this issue Sep 9, 2022
On some platform operator delete taints the memory, so even right after the deletion the information
stored by ~TObject is already erased.  On those platform we no longer rely on the kNotDelete bit hack
and rely on the system (which has tainted the memory assumingly for a reason) to detect the
use-after-delete problems.

Introduce 2 new functions.

TObject::IsDestructed (used by TClonesArray) that detects that the destructor has been run and is
active in all configuration.  This should be used if the code knows that the memory has not been
freed/deleted.

ROOT::Detail::HasBeenDeleted(TObject*) with returns true if the platform does not taint the memory
and if the kNotDeleted is not set (in all other case, it returns false)

This fixes root-project#11330
pcanal added a commit to pcanal/root that referenced this issue Sep 13, 2022
On some platform operator delete taints the memory, so even right after the deletion the information
stored by ~TObject is already erased.  On those platform we no longer rely on the kNotDelete bit hack
and rely on the system (which has tainted the memory assumingly for a reason) to detect the
use-after-delete problems.

Introduce 2 new functions.

TObject::IsDestructed (used by TClonesArray) that detects that the destructor has been run and is
active in all configuration.  This should be used if the code knows that the memory has not been
freed/deleted.

ROOT::Detail::HasBeenDeleted(TObject*) with returns true if the platform does not taint the memory
and if the kNotDeleted is not set (in all other case, it returns false)

This fixes root-project#11330
pcanal added a commit to pcanal/root that referenced this issue Sep 21, 2022
On some platform operator delete taints the memory, so even right after the deletion the information
stored by ~TObject is already erased.  On those platform we no longer rely on the kNotDelete bit hack
and rely on the system (which has tainted the memory assumingly for a reason) to detect the
use-after-delete problems.

Introduce 2 new functions.

TObject::IsDestructed (used by TClonesArray) that detects that the destructor has been run and is
active in all configuration.  This should be used if the code knows that the memory has not been
freed/deleted.

ROOT::Detail::HasBeenDeleted(TObject*) with returns true if the platform does not taint the memory
and if the kNotDeleted is not set (in all other case, it returns false)

This fixes root-project#11330
pcanal added a commit to pcanal/root that referenced this issue Sep 21, 2022
On some platform operator delete taints the memory, so even right after the deletion the information
stored by ~TObject is already erased.  On those platform we no longer rely on the kNotDelete bit hack
and rely on the system (which has tainted the memory assumingly for a reason) to detect the
use-after-delete problems.

Introduce 2 new functions.

TObject::IsDestructed (used by TClonesArray) that detects that the destructor has been run and is
active in all configuration.  This should be used if the code knows that the memory has not been
freed/deleted.

ROOT::Detail::HasBeenDeleted(TObject*) with returns true if the platform does not taint the memory
and if the kNotDeleted is not set (in all other case, it returns false)

This fixes root-project#11330
pcanal added a commit to pcanal/root that referenced this issue Sep 27, 2022
On some platform operator delete taints the memory, so even right after the deletion the information
stored by ~TObject is already erased.  On those platform we no longer rely on the kNotDelete bit hack
and rely on the system (which has tainted the memory assumingly for a reason) to detect the
use-after-delete problems.

Introduce 2 new functions.

TObject::IsDestructed (used by TClonesArray) that detects that the destructor has been run and is
active in all configuration.  This should be used if the code knows that the memory has not been
freed/deleted.

ROOT::Detail::HasBeenDeleted(TObject*) with returns true if the platform does not taint the memory
and if the kNotDeleted is not set (in all other case, it returns false)

This fixes root-project#11330
pcanal added a commit that referenced this issue Sep 28, 2022
On some platform operator delete taints the memory, so even right after the deletion the information
stored by ~TObject is already erased.  On those platform we no longer rely on the kNotDelete bit hack
and rely on the system (which has tainted the memory assumingly for a reason) to detect the
use-after-delete problems.

Introduce 2 new functions.

TObject::IsDestructed (used by TClonesArray) that detects that the destructor has been run and is
active in all configuration.  This should be used if the code knows that the memory has not been
freed/deleted.

ROOT::Detail::HasBeenDeleted(TObject*) with returns true if the platform does not taint the memory
and if the kNotDeleted is not set (in all other case, it returns false)

This fixes #11330
pcanal added a commit that referenced this issue Sep 28, 2022
On some platform operator delete taints the memory, so even right after the deletion the information
stored by ~TObject is already erased.  On those platform we no longer rely on the kNotDelete bit hack
and rely on the system (which has tainted the memory assumingly for a reason) to detect the
use-after-delete problems.

Introduce 2 new functions.

TObject::IsDestructed (used by TClonesArray) that detects that the destructor has been run and is
active in all configuration.  This should be used if the code knows that the memory has not been
freed/deleted.

ROOT::Detail::HasBeenDeleted(TObject*) with returns true if the platform does not taint the memory
and if the kNotDeleted is not set (in all other case, it returns false)

This fixes #11330
@pcanal pcanal added this to the 6.28/00 milestone Sep 28, 2022
vgvassilev pushed a commit to vgvassilev/root that referenced this issue Oct 1, 2022
On some platform operator delete taints the memory, so even right after the deletion the information
stored by ~TObject is already erased.  On those platform we no longer rely on the kNotDelete bit hack
and rely on the system (which has tainted the memory assumingly for a reason) to detect the
use-after-delete problems.

Introduce 2 new functions.

TObject::IsDestructed (used by TClonesArray) that detects that the destructor has been run and is
active in all configuration.  This should be used if the code knows that the memory has not been
freed/deleted.

ROOT::Detail::HasBeenDeleted(TObject*) with returns true if the platform does not taint the memory
and if the kNotDeleted is not set (in all other case, it returns false)

This fixes root-project#11330
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants