Do not initialize pointers to original functions in (libc_)free

[ldorau]

Description

Do not initialize pointers to original functions in (libc_)free(),
because dlsym() calls (libc_)free() internally, what can cause
the infinite recursion:
(libc_)free -> InitOrigPointers -> dlsym -> (libc_)free (see #1907).
This patch fixes this issue.

Fixes: #1907

Type of change

• bug fix - change that fixes an issue
• new feature - change that adds functionality
• tests - change in tests
• infrastructure - change in infrastructure and CI
• documentation - documentation update

Tests

• added - required for new features and some bug fixes
• not needed

Documentation

• updated in # - add PR number
• needs to be updated
• not needed

Breaks backward compatibility

• Yes
• No
• Unknown

Notify the following users

@Lastique

[ldorau]

@isaevil

[Lastique]

If I'm not mistaken, doInitOrigPointers is either always true or always false, depending on MALLOC_UNIXLIKE_OVERLOAD_ENABLED. Which means, doInitOrigPointers can be removed and this constructor should be empty unless MALLOC_UNIXLIKE_OVERLOAD_ENABLED is defined. If so, I would remove doInitOrigPointers and move the initOrigPointers class definition above the macro checks and define the constructor in the MALLOC_UNIXLIKE_OVERLOAD_ENABLED and MALLOC_ZONE_OVERLOAD_ENABLED sections.

[ldorau]

I removed doInitOrigPointers and used a preprocessor directive instead in order to keep the whole implementation in one place.
Done.

[ldorau]

@Lastique Does this patch fix your issue?

[Alexandr-Konovalov]

If I remember correctly, it was done via initialization on 1st use because some overloaded malloc were called before static ctor runs. In the suggested code, are we sure that initOrigPointers::free etc contain 0 because they sit in bss?

[Lastique]

@Lastique Does this patch fix your issue?

I didn't test this yet. I will probably have a chance to test it later this week.

If I remember correctly, it was done via initialization on 1st use because some overloaded malloc were called before static ctor runs. In the suggested code, are we sure that initOrigPointers::free etc contain 0 because they sit in bss?

The C++ standard guarantees that namespace-scope static variables are zero-initialized before constructors are run.

.bss sections are zero initialized on startup.

[ldorau]

@Lastique Does this patch fix your issue?

I didn't test this yet. I will probably have a chance to test it later this week.

OK. I have verified this fix using the reproducer provided by @Lastique in #1907 (comment) and it works:

root@dnp-02:/build/tbb/obj-x86_64-linux-gnu# make -j128 test ARGS\+=--verbose ARGS\+=-j128
[...]
100% tests passed, 0 tests failed out of 140

Total Test time (real) = 480.15 sec

root@dnp-02:/build/tbb/obj-x86_64-linux-gnu# ctest -I 137,140
Test project /build/tbb/obj-x86_64-linux-gnu
    Start 137: test_malloc_atexit
1/4 Test #137: test_malloc_atexit ...............   Passed    0.01 sec
    Start 138: test_malloc_overload
2/4 Test #138: test_malloc_overload .............   Passed    0.02 sec
    Start 139: test_malloc_overload_disable
3/4 Test #139: test_malloc_overload_disable .....   Passed    0.01 sec
    Start 140: test_malloc_new_handler
4/4 Test #140: test_malloc_new_handler ..........   Passed    0.01 sec

100% tests passed, 0 tests failed out of 4

Total Test time (real) =   0.05 sec

[Alexandr-Konovalov]

If I remember correctly, it was done via initialization on 1st use because some overloaded malloc were called before static ctor runs. In the suggested code, are we sure that initOrigPointers::free etc contain 0 because they sit in bss?

The C++ standard guarantees that namespace-scope static variables are zero-initialized before constructors are run.

.bss sections are zero initialized on startup.

So, free() is no-op, that is acceptable, but realloc allocates nothing before the static constructor executed. Can this be addressed somehow?

[ldorau]

If I remember correctly, it was done via initialization on 1st use because some overloaded malloc were called before static ctor runs. In the suggested code, are we sure that initOrigPointers::free etc contain 0 because they sit in bss?

The C++ standard guarantees that namespace-scope static variables are zero-initialized before constructors are run.
.bss sections are zero initialized on startup.

So, free() is no-op, that is acceptable, but realloc allocates nothing before the static constructor executed. Can this be addressed somehow?

If no-op free() is acceptable then there is a much simpler fix that addresses the above mentioned issue.
@Alexandr-Konovalov @lplewa @KFilipek please re-review.

[Lastique]

Note that the original code is not thread-safe, as accesses to orig_free etc. are not atomic (i.e. readers may observe a partially stored value of the function pointer). The previous version of the fix was also not quite thread-safe but it at least initialized the pointers during global initialization stage, where there are unlikely multiple threads running.

This doesn't mean this has to be fixed in this PR, just an observation.

Also, one other alternative to consider is the init_priority attribute that can be used to force early initialization of the function pointers.

[ldorau]

Note that the original code is not thread-safe, as accesses to orig_free etc. are not atomic (i.e. readers may observe a partially stored value of the function pointer).

@Lastique There is a comment: // race is OK here, as different threads found same functions in InitOrigPointers().
I think the authors meant that the first __TBB_malloc_safer_*() function will be called when the first call to InitOrigPointers() is finished, so when all the pointers: orig_free, orig_realloc, orig_msize, orig_libc_free, orig_libc_realloc are set to the correct values. In meantime another thread can be storing those pointers again and one of those pointers can be "partially stored", but it does not matter, because a new value (partially stored) is equal to the old value, so a reader will read the correct value even if it is "partially stored", because the rest of the value (not-yet-stored) is already correct (stored by the thread that already returned from InitOrigPointers()).

[ldorau]

@Alexandr-Konovalov @lplewa @KFilipek please re-review.

[Alexandr-Konovalov]

LGTM.

[ldorau]

@Alexandr-Konovalov @isaevil Can you merge it?

[Lastique]

My local TBB package build with the current version of this PR applied passed. Thanks.