jupyter-xeus / cpp-terminal

C++ library for writing multiplatform terminal applications
https://jupyter-xeus.github.io/cpp-terminal/
Other
490 stars 53 forks source link

Exception thrown at this line when typeing to fast #301

Closed TobiasWallner closed 11 months ago

TobiasWallner commented 11 months ago

https://github.com/jupyter-xeus/cpp-terminal/blob/52699fd3d2d0342ffa7b358e3f8ebd69dd9042f4/cpp-terminal/event.cpp#L417C60-L417C60

That line throws the following exception when I am typeing too fast or useing multibyte utf8 characters_

Unhandled exception at 0x00007FFFAE3A024E (ucrtbased.dll) in lime.exe: 0xC0000005: Access violation reading location 0xFFFFFFFFFFFFFFFF.

Why are we using a std::unique pointer and allocate a new string on every unicode character that is longer than one byte here? At least I will enter this code block whenever I hit one. Seems a little inefficient to me. Why did we move away from the std::string that just keeps its last allocated storage?

it seems like at some point in the std::unique_ptr::reset() function the following code will be invoked:

_CRT_SECURITYCRITICAL_ATTRIBUTE
void __CRTDECL operator delete(void* const block) noexcept
{
    #ifdef _DEBUG
    _free_dbg(block, _UNKNOWN_BLOCK);
    #else
    free(block);
    #endif
}

and _free_dbg(block, _UNKNOWN_BLOCK); will then throw and probably free(block); is throwing in Release builds because it happens in Debug and Release builds. Happens on GCC, Clang and MSVC. It is very hard for me to figure out more since I beliefe that this exception is comeing from a detatched thread that reads the input.

certik commented 11 months ago

That code:

      m_Type = Type::CopyPaste;
      m_container.m_string.reset(new char[str.size() + 1]);
      std::copy(str.begin(), str.end(), m_container.m_string.get());
      m_container.m_string.get()[str.size()] = '\0';
      return;

should not be written like this. First of all, one should not use "new" by hand like this, since it usually leads to leaks and segfaults, possibly even causing the issue that you've seen. Second, as you said, this doesn't seem efficient. Why can't we simply append to the string?

TobiasWallner commented 11 months ago

Why can't we simply append to the string?

or assign

flagarde commented 11 months ago

@certik @TobiasWallner I fully agree with all your comments. Sorry for this stupid change, will move back to string

certik commented 11 months ago

@flagarde awesome, thanks! No worries, this should be easy to fix.

flagarde commented 11 months ago

@certik @TobiasWallner Should be fixed now

flagarde commented 11 months ago

I tested quickly on linux but please have a try too because string inside union it's a bit tricky

TobiasWallner commented 11 months ago

I just tried it and it works like a charm again, thx