Изменение типа переменной в конструкторе и деструкторе - это что за? , Баг в Bonzomatic

[xintrea]

Собрал в Debian Linux программу Bonzomatic из git-репозитария https://github.com/Gargaj/Bonzomatic

Если ее запустить, переключиться на русскую раскладку, и нажать любую русскую букву, то произойдет сегфолт:

*** Error in `./bonzomatic': free(): invalid next size (fast): 0x000056048520f980 ***
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(+0x70bfb)[0x7fa3c7f6ebfb]
/lib/x86_64-linux-gnu/libc.so.6(+0x76fc6)[0x7fa3c7f74fc6]
/lib/x86_64-linux-gnu/libc.so.6(+0x7780e)[0x7fa3c7f7580e]
./bonzomatic(_ZN9Scintilla10LineLayout4FreeEv+0x7b)[0x56048373ba39]
./bonzomatic(_ZN9Scintilla10LineLayoutD2Ev+0x26)[0x56048373b8ea]
./bonzomatic(_ZN9Scintilla10LineLayoutD0Ev+0x18)[0x56048373b906]
./bonzomatic(_ZN9Scintilla15LineLayoutCache8RetrieveEiiiiii+0x21c)[0x56048373c920]
./bonzomatic(_ZN9Scintilla8EditView18RetrieveLineLayoutEiRKNS_9EditModelE+0x143)[0x56048372377f]
./bonzomatic(_ZN9Scintilla6Editor11WrapOneLineEPNS_7SurfaceEi+0x30)[0x560483702c9c]
./bonzomatic(_ZN9Scintilla6Editor9WrapLinesENS0_9wrapScopeE+0x487)[0x56048370323f]
./bonzomatic(_ZN9Scintilla6Editor5PaintEPNS_7SurfaceENS_10PRectangleE+0x10e)[0x560483703e88]
./bonzomatic(_ZN12ShaderEditor5PaintEv+0xa5)[0x56048363edc5]
./bonzomatic(main+0x463e)[0x56048364641e]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf1)[0x7fa3c7f1e2e1]
./bonzomatic(_start+0x2a)[0x56048362adaa]
======= Memory map: ========

В этой программе есть простейший тип:

typedef float XYPOSITION;

И у класса LineLayout есть публичное свойство, в виде указателя на этот тип:

class LineLayout {
  ...
  XYPOSITION *positions;
  ...
}

И вот с этим свойством в дебаггере творится лютая дичь: у него меняется тип. В конструкторе у этой переменной тип правильный, такой, какой объявлен в заголовке. А в деструкторе у этой же переменной тип становится "простым", то есть без указателя. Я специально сделал GIF-анимацию из двух скриншотов:

Картинка с GIF-анимацией

То есть, даже до сегфолта, если поставить брекпоинт в конструктор и деструктор, будет постоянно наблюдатся вот это странное изменени типа.

Ну и дальше, сегфолт появляется на строчке:

void LineLayout::Free() {
    delete []chars;
    chars = 0;
    delete []styles;
    styles = 0;
    delete []positions; // <-- Тут сегфолт
    positions = 0;
    delete []lineStarts;
    lineStarts = 0;
}

Примечательно, что во время сегфолта, если просмотреть скоуп сегфолта, то у переменных chars, styles, lineStarts тип остается тем же, а у positions - слетает.

Как это вообще возможно, я понять не могу. В общем, по всей видимости, происходит двойное высвобождение ресурса positions, но вот это вот странное изменение типа в строго типизированном языке, не дает возможности отследить что блин вообще происходит, и в какой момент происходит первое высвобождение.

Поможите пофиксить этот баг.

[Qraizer]

С тем же успехом это может быть порча хипа превышением границ другого массива. И я даже подозреваю, что правильно предположу причину: UTF-8.

P.S. С кодом разбираться в режиме оффлайн то ещё удовольствие. Я бы предложил тебе его сначала в порядок привести. Один только фрагмент

void LineLayout::Resize(int maxLineLength_) {
    if (maxLineLength_ > maxLineLength) {
        Free();
        chars = new char[maxLineLength_ + 1];
        styles = new unsigned char[maxLineLength_ + 1];
        // Extra position allocated as sometimes the Windows
        // GetTextExtentExPoint API writes an extra element.
        positions = new XYPOSITION[maxLineLength_ + 1 + 1];
        maxLineLength = maxLineLength_;
    }
}

Мало того, что код абсолютно не отказоустойчив (лично мне этого вполне достаточно, чтобы выкинуть эту репу в корзину и больше никогда к ней не возвращаться), в таком стиле на Плюсах уже 23 года не пишут. Перепиши на контейнерах, включи отладочный режим, и STL сама тебе об авторе этого кода всё расскажет.

Сообщение отредактировано: Qraizer - 24.06.21, 18:35

[xintrea]

Цитата Qraizer @ 24.06.21, 18:33

Дык вроде Scintilla умеет работать с UTF-8, и в Bonzomatic даже выставлено:

WndProc( SCI_SETCODEPAGE, SC_CP_UTF8, 0 );

Вроде должно же правильно работать.

Я конечно попробую только это поле сделать контейнером, может быть хватит ума сделать аналог.

[ЫукпШ]

А если так:

    void LineLayout::Free()
    {
        if(chars)      { delete []chars;      chars     = 0; }
        if(styles)     { delete []styles;     styles    = 0; }
        if(positions)  { delete []positions;  positions = 0;  }  // <-- Тут сегфолт
        if(lineStarts) { delete []lineStarts; lineStarts = 0; }

И понятно, что все указатели до применения LineLayout::Free() {в конструкторе ?)
должны буть правильно инициализированы.

Сообщение отредактировано: ЫукпШ - 24.06.21, 19:35

[shm]

ЫукпШ, Стандарт гарантирует, что для nullptr деструкторы и free вызываться не будут.

[ЫукпШ]

Цитата shm @ 24.06.21, 19:38

ЫукпШ, Стандарт гарантирует, что для nullptr деструкторы и free вызываться не будут.

я знаю, но однажды при попытке делать так для Linux (Fedora),
приложение устойчиво падало. И было не просто понять, с чего это вдруг.
Для Виндуса такого действительно не наблюдается.

[shm]

Судя по падению - повреждена куча. Во-первых https://en.wikipedia.org/wiki/Memory_debugger. Во-вторых Qraizer все правильно сказал, что ручное управление памятью - это плохо (за исключением некоторых редких специфических задач вроде написания своего аллокатора).

Добавлено 24.06.21, 19:50

Цитата ЫукпШ @ 24.06.21, 19:45

я знаю, но однажды при попытке делать так для Linux (Fedora),
приложение устойчиво падало.

Честно - не поверю пока сам не увижу:)

Сообщение отредактировано: shm - 24.06.21, 19:50

[xintrea]

Цитата Qraizer @ 24.06.21, 18:33

В общем, если запустить под Valgrind и опустить ошибки в libnvidia-glcore.so и libGLX_nvidia.so, то получится следующее:

==5074== Invalid write of size 4
==5074==    at 0x1EFDC3: Scintilla::SurfaceImpl::MeasureWidths(Scintilla::Font&, char const*, int, float*) (Platform.cpp:450)
==5074==    by 0x2F3033: Scintilla::PositionCache::MeasureWidths(Scintilla::Surface*, Scintilla::ViewStyle const&, unsigned int, char const*, unsigned int, float*, Scintilla::Document*) (PositionCache.cxx:680)
==5074==    by 0x2D92F3: Scintilla::EditView::LayoutLine(Scintilla::EditModel const&, int, Scintilla::Surface*, Scintilla::ViewStyle const&, Scintilla::LineLayout*, int) (EditView.cxx:477)
==5074==    by 0x2B7D22: Scintilla::Editor::WrapOneLine(Scintilla::Surface*, int) (Editor.cxx:1386)
==5074==    by 0x2B823E: Scintilla::Editor::WrapLines(Scintilla::Editor::wrapScope) (Editor.cxx:1461)
==5074==    by 0x2B8E87: Scintilla::Editor::Paint(Scintilla::Surface*, Scintilla::PRectangle) (Editor.cxx:1633)
==5074==    by 0x1F3DC4: ShaderEditor::Paint() (ShaderEditor.cpp:281)
==5074==    by 0x1FB41D: main (main.cpp:549)
==5074==  Address 0xfba6d9c is 0 bytes after a block of size 28 alloc'd
==5074==    at 0x4C2C93F: operator new[](unsigned long) (vg_replace_malloc.c:423)
==5074==    by 0x2F099D: Scintilla::LineLayout::Resize(int) (PositionCache.cxx:83)
==5074==    by 0x2F08C0: Scintilla::LineLayout::LineLayout(int) (PositionCache.cxx:69)
==5074==    by 0x2F197D: Scintilla::LineLayoutCache::Retrieve(int, int, int, int, int, int) (PositionCache.cxx:349)
==5074==    by 0x2D877E: Scintilla::EditView::RetrieveLineLayout(int, Scintilla::EditModel const&) (EditView.cxx:348)
==5074==    by 0x2B7C9B: Scintilla::Editor::WrapOneLine(Scintilla::Surface*, int) (Editor.cxx:1383)
==5074==    by 0x2B9D66: Scintilla::Editor::AddCharUTF(char const*, unsigned int, bool) (Editor.cxx:1836)
==5074==    by 0x1F3FCC: ShaderEditor::AddCharUTF(char const*, unsigned int, bool) (ShaderEditor.cpp:314)
==5074==    by 0x1FAF78: main (main.cpp:484)
==5074==
--5074-- VALGRIND INTERNAL ERROR: Valgrind received a signal 11 (SIGSEGV) - exiting
--5074-- si_code=128;  Faulting address: 0x0;  sp: 0x80316de20
 
valgrind: the 'impossible' happened:
   Killed by fatal signal
 
host stacktrace:
==5074==    at 0x38091D1F: get_bszB_as_is (m_mallocfree.c:300)
==5074==    by 0x38091D1F: get_bszB (m_mallocfree.c:311)
==5074==    by 0x38091D1F: vgPlain_arena_free (m_mallocfree.c:2044)
==5074==    by 0x3804F963: release_oldest_block (mc_malloc_wrappers.c:165)
==5074==    by 0x3804F963: create_MC_Chunk (mc_malloc_wrappers.c:208)
==5074==    by 0x3804FB2B: vgMemCheck_new_block (mc_malloc_wrappers.c:366)
==5074==    by 0x3804FCA6: vgMemCheck_malloc (mc_malloc_wrappers.c:385)
==5074==    by 0x380D7B53: do_client_request (scheduler.c:1866)
==5074==    by 0x380D7B53: vgPlain_scheduler (scheduler.c:1425)
==5074==    by 0x380E6416: thread_wrapper (syswrap-linux.c:103)
==5074==    by 0x380E6416: run_a_thread_NORETURN (syswrap-linux.c:156)
 
sched status:
  running_tid=1
 
Thread 1: status = VgTs_Runnable (lwpid 5074)
==5074==    at 0x4C2BBAF: malloc (vg_replace_malloc.c:299)
==5074==    by 0xD236D68: ??? (in /usr/lib/x86_64-linux-gnu/nvidia/current/libGLX_nvidia.so.375.82)
==5074==    by 0xE6F88E6: ??? (in /usr/lib/x86_64-linux-gnu/libnvidia-glcore.so.375.82)
==5074==    by 0xE369048: ??? (in /usr/lib/x86_64-linux-gnu/libnvidia-glcore.so.375.82)
==5074==    by 0xE34E214: ??? (in /usr/lib/x86_64-linux-gnu/libnvidia-glcore.so.375.82)
==5074==    by 0xE34FF15: ??? (in /usr/lib/x86_64-linux-gnu/libnvidia-glcore.so.375.82)
==5074==    by 0x21BF54: Renderer::__FlushRenderCache() (Renderer.cpp:875)
==5074==    by 0x21C224: Renderer::BindTexture(Renderer::Texture*) (Renderer.cpp:928)
==5074==    by 0x1EF83F: Scintilla::SurfaceImpl::DrawTextBase(Scintilla::PRectangle, Scintilla::Font&, float, char const*, int, Scintilla::ColourDesired) (Platform.cpp:381)
==5074==    by 0x1EFCAD: Scintilla::SurfaceImpl::DrawTextTransparent(Scintilla::PRectangle, Scintilla::Font&, float, char const*, int, Scintilla::ColourDesired) (Platform.cpp:423)
==5074==    by 0x2DFCFF: Scintilla::EditView::DrawForeground(Scintilla::Surface*, Scintilla::EditModel const&, Scintilla::ViewStyle const&, Scintilla::LineLayout const*, int, Scintilla::PRectangle, Scintilla::Range, int, int, int, Scintilla::ColourOptional) (EditView.cxx:1514)
==5074==    by 0x2E0DAB: Scintilla::EditView::DrawLine(Scintilla::Surface*, Scintilla::EditModel const&, Scintilla::ViewStyle const&, Scintilla::LineLayout const*, int, int, int, Scintilla::PRectangle, int, Scintilla::DrawPhase) (EditView.cxx:1676)
==5074==    by 0x2E1BFF: Scintilla::EditView::PaintText(Scintilla::Surface*, Scintilla::EditModel const&, Scintilla::PRectangle, Scintilla::PRectangle, Scintilla::ViewStyle const&) (EditView.cxx:1835)
==5074==    by 0x2B91B0: Scintilla::Editor::Paint(Scintilla::Surface*, Scintilla::PRectangle) (Editor.cxx:1678)
==5074==    by 0x1F3DC4: ShaderEditor::Paint() (ShaderEditor.cpp:281)
==5074==    by 0x1FB41D: main (main.cpp:549)
 
Thread 2: status = VgTs_WaitSys (lwpid 5116)
==5074==    at 0x5D8320D: ??? (syscall-template.S:84)
==5074==    by 0x8AC1180: pa_read (in /usr/lib/x86_64-linux-gnu/pulseaudio/libpulsecommon-10.0.so)
==5074==    by 0x886E0BD: pa_mainloop_prepare (in /usr/lib/x86_64-linux-gnu/libpulse.so.0.20.1)
==5074==    by 0x886EB2F: pa_mainloop_iterate (in /usr/lib/x86_64-linux-gnu/libpulse.so.0.20.1)
==5074==    by 0x229639: ma_device_read__pulse(ma_device*, void*, unsigned int, unsigned int*) (miniaudio.h:20250)
==5074==    by 0x229C13: ma_device_main_loop__pulse(ma_device*) (miniaudio.h:20405)
==5074==    by 0x22D268: ma_worker_thread(void*) (miniaudio.h:29561)
==5074==    by 0x5D7A493: start_thread (pthread_create.c:333)
==5074==    by 0x6915ACE: clone (clone.S:97)

[Qraizer]

Цитата xintrea @ 24.06.21, 18:46

Я конечно попробую только это поле сделать контейнером, может быть хватит ума сделать аналог.

Там всё надо на контейнеры переводить. Портить хип может выход за границы любого массива.
И я не зря предложил привести весь код в порядок. На вот, пожалуйста:

class PositionCache {
    std::vector<PositionCacheEntry> pces;
...
};
 
/* ... */
 
class PositionCacheEntry {
    unsigned int styleNumber:8;
    unsigned int len:8;
    unsigned int clock:16;
    XYPOSITION *positions;
public:
    PositionCacheEntry();
    ~PositionCacheEntry();
    void Set(unsigned int styleNumber_, const char *s_, unsigned int len_, XYPOSITION *positions_, unsigned int clock_);
    void Clear();
    bool Retrieve(unsigned int styleNumber_, const char *s_, unsigned int len_, XYPOSITION *positions_) const;
    static unsigned int Hash(unsigned int styleNumber_, const char *s, unsigned int len);
    bool NewerThan(const PositionCacheEntry &other) const;
    void ResetClock();
};
 
PositionCacheEntry::~PositionCacheEntry() {
    Clear();
}
 
void PositionCacheEntry::Clear() {
    delete []positions;
    positions = 0;
    styleNumber = 0;
    len = 0;
    clock = 0;
}

Что будет, если PositionCache::pces, будучи вектором, захочет переаллоцировать память? Правильно: он захочет покопировать/поприсваивать свои элементы PositionCacheEntry. У них нет конструктора копии, у них нет операции присваивания, поэтому будут задействованы стандартные, которые просто покопируют указатели PositionCacheEntry::positions. Но есть деструктор, который вызовет PositionCacheEntry::Clear(), который сделает delete []positions. Приехали.
Вот ещё:

class LineLayoutCache {
...
        virtual ~LineLayoutCache();
...
};

Внимание, вопрос знатокам: нахрена ему виртуальный деструктор, если это единственный виртуальный метод, а LineLayoutCache не предназначен быть базовым и даже никакого интерфейса для производных классов не документирует?
В общем, код нужно знатно рефакторить. Его автор явно не в ладах с Плюсами. Не понимает, зачем нужен полиморфизм, и даже неправильно умеет контейнеры.

Сообщение отредактировано: Qraizer - 24.06.21, 20:31

[xintrea]

Цитата ЫукпШ @ 24.06.21, 19:32

Я условия на 0 сразу же добавил, но проблема в другом.

По Valgrind, проблемное место следующее:

void SurfaceImpl::MeasureWidths(Font & font, const char *str, int len, float *positions)
{
  stbtt_Font* realFont = (stbtt_Font*)font.GetID();
  
  float position = 0;
  const char * p = str;
  while (len-- > 0)
  {
    unsigned int c = *p;
    unsigned int charLength = UTF8CharLength(c);
    if (charLength > 1)
    {
      c = 0;
      UTF16FromUTF8( str, charLength, (wchar_t*)&c, sizeof(unsigned int) );
    }
    if (c >= CHARACTER_COUNT)
      c = '?';
 
    int advance = 0, leftBearing = 0;
    
    stbtt_GetCodepointHMetrics(&realFont->fontinfo, c, &advance, &leftBearing);
    
    position     += advance;
    for (unsigned int i=0; i<charLength; i++) // we need to loop here because UTF8 characters count as multiple unless their position is the same
      *positions++  = position*realFont->scale;
 
    p += charLength;
    len -= charLength - 1;
  }
}

Внутри цикла for() следующая команда пишет в память мимо кассы:

*positions++  = position*realFont->scale;

Пока что не понял, как оно правильно должно быть.

[OpenGL]

Цитата Qraizer @ 24.06.21, 18:33

в таком стиле на Плюсах уже 23 года не пишут.

Почему именно 23? До 98 года не было деструкторов?

Добавлено 25.06.21, 06:58

Цитата Qraizer @ 24.06.21, 20:29

На вот, пожалуйста:

Забавно Но конкретно в данном случае это, видимо, не является проблемой. Размер psec задаётся либо в конструкторе, либо в методе SetSize, и больше не меняется нигде. В первом случае объекты и так пустые, во втором перед ресайзом вызывается полная очистка.

[amk]

Цитата OpenGL @ 25.06.21, 06:34

Почему именно 23? До 98 года не было деструкторов?

До 98 года в составе стандарта не было stl. Поэтому не было безопасных контейнеров и приходилось всё делать вручную.

[Qraizer]

Цитата OpenGL @ 25.06.21, 06:34

Но конкретно в данном случае это, видимо, не является проблемой. Размер psec задаётся либо в конструкторе, либо в методе SetSize, и больше не меняется нигде.

Неважно. Вот начнёт счас xintrea код в нормальный вид приводить, запросто отхватит. И я-т ведь просто поискал возможные причины, телепатически т.с., почему безопасные конструкции могут фэйлнуть и дважды вызвать функцию очистки. И на́ тебе, наткнулся на грубое нарушение требования STL класть в контейнеры только объекты, обладающими семантикой значений. А если копнуть поглубже?

Цитата OpenGL @ 25.06.21, 06:34

В первом случае объекты и так пустые, во втором перед ресайзом вызывается полная очистка.

Это ещё хуже. Представь себе, выделил ты такой массив вертексов мышой, скомандовал "увеличить полиномность фигуры впятеро и сгладить новые вертексы сплайном" (синтетический пример, конечно) и наблюдаешь, как все вертексы внезапно исчезли, а полиномы, что на них опирались, свалились в бесформенную кучу. Грубейшее нарушение отказоустойчивости: при создании нового состояния старое удаляется до того, как новое будет успешно получено. В итоге при ошибке нового состояния получить не удалось, а старое уже уничтожено. От кто так пишет, а? Ну разве что я 20 лет назад.

Сообщение отредактировано: Qraizer - 29.06.21, 18:59