mattiasflodin/reckless

Probably problem with SSO for std::string.

Closed this issue · 6 comments

itwasntme-mk commented

Code:

    std::string str("~~~~~~~~~~~~~~~");

    for (int i = 0; i != 1000000; ++i)
      g_log.info("Info line: %s @%d", str, i);

Result in:

munmap_chunk(): invalid pointer
Aborted

Randomly I saw also:

munmap_chunk(): invalid size

If str has more than 15 characters problem doesn't appear. @mattiasflodin Could you take a look at that? Probably it is similar to #35 (also short string).

mattiasflodin commented

Hi, just wanted to let you know I've reproduced this and I'll have a look at it. Thanks for the report.

serpent7776 commented

Is there any progress on this?

I noticed that increasing input_buffer_capacity when creating basic_log instance reduces chance of getting crash.

@mattiasflodin Are you sure using relaxed atomics is ok here? https://github.com/mattiasflodin/reckless/blob/v3.0.2/reckless/include/reckless/detail/platform.hpp#L76
Wouldn't it be better to use std::atomic?
The TODO note there is in particular concerning.

mopleen commented

Maybe this demo will be useful https://github.com/mopleen/reckless_demo_crash
What's interesting is that the crash does not happen on the system running libc 2.17

Also, logging only long strings does not crash either. Short string is what's triggering the problem.

mattiasflodin commented

@serpent7776 @mopleen Thank you for your patience with this bug and your efforts to provide reproduction demos and inspecting the code. I have been busy with some other projects so this had to be put on hold for a while. Anyway, I believe I have figured out what is going wrong. The usual suspect would of course be multithreading and incorrect use of atomics, but in fact this error is not related to that.

The problem lies in the ring buffer, which is implemented by mapping multiple virtual addresses to the same physical memory in an attempt to avoid some wraparound checks. basic_string in libstdc++ has an optimization for strings of 16 characters or less, in which it stores the data in a local array instead of allocating memory. When the string is destroyed, it performs this comparison to determine whether the memory should be deallocated or not:

_M_dispose()
{
    if (!_M_is_local())
        _M_destroy(_M_allocated_capacity);
}

_M_is_local() operates by comparing the buffer pointer to the address of the internal array. If they don't match, it assumes that the memory was allocated and attempts to deallocate it.

For some reason that I'm currently looking into, the same virtual address is not being used when the object is destroyed as when it is created (even though they refer to the same physical memory), which causes _M_is_local to return false and basic_string then attempts to free a buffer that was never allocated.

It may turn out that my ring buffer is too clever for its own good and that I need to change its implementation. I hope not, because that could make the performance of pushing very large objects unintuitive. But so far I believe there is just a bug in some pointer arithmetic related to the buffer.

  • 👍1
mattiasflodin commented

Resolved in v3.0.3.

  • 🎉1
serpent7776 commented

Seems it's working fine now, thanks.

  • 👍1