Bugs in GoldEd

Nil Alexandrov написал(а) к All в May 21 06:07:18 по местному времени:

Нello, All!

Правильно я понимаю, что последняя версия golded лежит https://github.com/golded-plus/golded-plus ?

Далее следуют мои наблюдения.

1. char buf[256], options[80];
Много функций имеют такие вот фиксированные буфера на стеке для работы с 80x25 терминалом, если запускать в Linux/Mac широком терминале, то происходит переполнение.
Быстрое лечение - замена на бОльшие значения буферов.
Правильное лечение - узнать размер терминала, выделить буфер соответственно, ещё надо ловить сигналы, когда размер терминала меняется и перевыделять буфера и перерисовывать экран по-хорошему - это много кода и он платформенно зависимый.

2. Overflow
Прочие переполнения фиксированных буферов, например, когда идёт цветовое выделение фрагмента, можно предположить худший сценарий, когда весь текст будет выделен.
-+- a/golded3/gectnr.cpp
+++ b/golded3/gectnr.cpp
@@ -95,7 +95,7 @@ void Container::StyleCodeНighlight(const char* text, int row, int col, bool dohi

uint sclen = 0;
const char* txptr = text;
- char buf[200];
+ char buf = (char)throw_calloc(1, strlen(text)+1);
const char* ptr = text;
const char* stylemargins = " -|\\"; // we probably have to make a keyword for it
char* punctchars = CFG->stylecodepunct;

3. New delete type mismatch
Есть места, где буфер выделают под массив, а освобождают как переменную, например
-+- a/golded3/gearea.cpp
+++ b/golded3/gearea.cpp
@@ -151,8 +151,8 @@ void GPickArealist::do_delayed()
LOG.printf("! Buffer overflow: tmp in GPickArealist::do_delayed(), 15 or 16 lines above");
PointerErrorExit();
}
- delete buf;
- delete tmp;
+ delete[] buf;
+ delete[] tmp;
}


-+- a/golded3/geinit.cpp
+++ b/golded3/geinit.cpp
@@ -536,7 +536,7 @@ void Uninitialize()
{
if (WideUsername)
{
- delete WideUsername;
+ delete[] WideUsername;
WideUsername = NULL;
}
LookupNodeClear();

4. Strcpy param overlap
Есть код, где нужно удалить часть строки, например начальные Re: - это реализуется путём копирования подстроки. Во многих местах для этого используется strcpy(), тогда как для перекрывающихся участков памяти нужно использовать memmove.
-+- a/golded3/gepost.cpp
+++ b/golded3/gepost.cpp
@@ -1076,7 +1076,7 @@ void MakeMsg(int mode, GMsg* omsg, bool ignore_replyto)
ptr = strskip_wht(r);
}
while(r);
- strcpy(msg->re, ptr);
+ memmove(msg->re, ptr, strlen(ptr)+1);
}
strcpy(msg->ddom, omsg->odom);
if((mode == MODE_REPLYCOMMENT) and omsg->fwdorig.net)
-+- a/golded3/getpls.cpp
+++ b/golded3/getpls.cpp
@@ -350,17 +350,17 @@ int TemplateToText(int mode, GMsg msg, GMsg* oldmsg, const char tpl, int origa

// build @dpseudo
if(msg->to_me())
- strcpy(oldmsg->pseudoto, msg->pseudofrom);
+ memmove(oldmsg->pseudoto, msg->pseudofrom, strlen(msg->pseudofrom)+1);
else if(msg->to_you())
- strcpy(oldmsg->pseudoto, msg->pseudoto);
+ memmove(oldmsg->pseudoto, msg->pseudoto, strlen(msg->pseudoto)+1);
else
*(oldmsg->pseudoto) = NUL;

// build @opseudo
if(msg->by_me())
- strcpy(oldmsg->pseudofrom, msg->pseudofrom);
+ memmove(oldmsg->pseudofrom, msg->pseudofrom, strlen(msg->pseudofrom)+1);
else if(msg->by_you())
- strcpy(oldmsg->pseudofrom, msg->pseudoto);
+ memmove(oldmsg->pseudofrom, msg->pseudoto, strlen(msg->pseudoto)+1);
else
*(oldmsg->pseudofrom) = NUL;

5. Memset() clearing an object with no trivial copy-assignment.
С многими C++ объектами работают, как если бы это были C-структуры с POD типами, например, инициализация объекта сообщений происходит так memset(msg, 0, sizeof(GMsg)), хотя это наследуемый класс от gmsg, и там есть много сложных объектов типа ftnaddr, или даже gmsg_links, внутри которого есть std::vector. Также, с целью оптимизации выделения/освобождения памяти, переиспользуют такие объекты, просто забивая их нулями. Благодаря счастливому стечению обстоятельств, нули работают как и для char*, и для ftnaddr и даже, скорее всего, для локальной реализации std::vector, который хранит указатель на буфер и размер. Что точно ломается - это память, которую выделяет внутри std::vector.
По хорошему, надо сделать рефакторинг, чтобы инициализировать объекты C++ соответствующими для этого методами.
Быстрый хак, чтобы не утекала память под std::vector, работает только начиная с C++11.
-+- a/goldlib/gmb3/gmo_msg.h
+++ b/goldlib/gmb3/gmo_msg.h
@@ -250,6 +250,7 @@ public:
{
replyto = reply_first = replynext = 0;
reply_list.clear();
+ replylist.shrink_tofit();
}

int list_max() const

6. Memory leaks
Наверное их много, сходу ловиться вот такой.
-+- a/goldlib/gall/gespell.cpp
+++ b/goldlib/gall/gespell.cpp
@@ -834,6 +834,9 @@ void CSpellChecker::Close()
{
if (!IsInited()) return;
UnLoad();
+ CSpellLangV::iterator it;
+ for (it = mLangs.begin(); it != mLangs.end(); it++)
+ delete *it;
mLangs.clear();
mInited = false;
}

7. ISO C++ forbids converting a string constant to char*
Все строковые константы из .text сегмента передаются дальше по коду без константы, что провоцирует много предупреждений со стороны компилятора. Хочется надеяться, что ни какой код не начёт переписать содержимое таких буферов.

Best Regards, Nil
--- GoldED+/LNX 1.1.5