PDA

Просмотр полной версии : [Info] Баг в GetPVarString / GetSVarString и как его исправить



Daniel_Cortez
16.12.2015, 20:25
Всем привет.
Начну с того, что это не совсем обычный урок - скорее, повествование. Но нигде на pro-pawn я не видел статей подобного жанра, так что...


Всё началось с того, что Untonyst (http://pro-pawn.ru/member.php?u=4123), проверяя работу своего инклуда fix_K2BEx.inc (http://pro-pawn.ru/showthread.php?12832-fix_K2BEx-inc), наткнулся на странный баг: при использовании функции BanEx в samp.ban вместо текста бана записывалась какая-то "каша" из букв.
Об этом он вскоре написал мне и, как оказалось, баг наблюдался и в моём инклуде dc_kickfix.inc (http://pro-pawn.ru/showthread.php?12837-dc_kickfix-inc), поэтому я решил разобраться.

Для начала я взял тестовый мод, в котором использовался инклуд dc_kickfix.inc, и ввёл в нём команду "/ban 0 проверка":
http://ihost.pro-pawn.ru/image.php?di=YWF7

А вот содержимое файла samp.ban после бана:
http://ihost.pro-pawn.ru/image.php?di=NXMW

Пример кода, с помощью которого можно воспроизвести этот баг:


SetPVarString(playerid, "ban_reason", "проверка");
new reason[128];
GetPVarString(playerid, "ban_reason", reason, sizeof(reason));
DeletePVar(playerid, "ban_reason");
BanEx(playerid, reason);

Поскольку задачей инклудов fix_K2BEx.inc и dc_kickfix.inc было сделать кик/бан с задержкой, чтобы успеть показать игроку причину, функции Kick, Ban и BanEx вызывались с помощью таймера.
А чтобы как-то передать функции BanEx строку с причиной бана (в функции SetTimerEx спецификатор "s" не работает), эта строка предварительно сохранялась в PVar.
Можно было бы сделать сохранение в одномерном массиве, но если забанить 2 игроков за полсекунды, то последний игрок перезапишет причину бана первого (массив-то общий!) и первому игроку будет показана та же причина бана, что и последнему.
Если же сделать двухмерный массив, то его придётся делать размером MAX_PLAYERS * 128 ячеек (вместо 128 может быть и меньшее число - главное, чтобы вместилась причина бана), но резервировать участок памяти сразу под всех игроков и никогда не высвобождать его тоже не целесообразно.
Именно поэтому было решено использовать PVar'ы - с ними память выделяется только тогда, когда это нужно, а после использования её можно высвободить, удалив PVar.

Со слов Untonyst, баг проявлялся только в тех случаях, когда в причине бана были русские буквы, и если в коде выше поставить "print(reason);" перед вызовом BanEx, то причина бана с символами кириллицы будет выведена, как ни в чём не бывало, но в samp.ban текст запишется неправильно. Очень странно.
С его же слов, если вместо хранения причины в PVar сделать двухмерный массив и сохранять причину бана в нём, то никаких проблем с сохранением русского текста в samp.ban не будет.
Значит причина, скорее всего, кроется в PVar'ах.
Я решил проверить эту догадку, но только на SVar'ах, чтобы можно было провести тесты на пустом сервере. Всё равно механизм хранения в SVar'ах примерно тот же самый, разве что без привязки к игрокам.


#include <a_samp>

main()
{
static str[] = "AaBbZzАаБбЯя";
new str1[20], str2[20];
str1[0] = '\0', strcat(str1, str, sizeof(str1));
SetSVarString("reason", str);
GetSVarString("reason", str2, sizeof(str2));
print(str1);
print(str2);
for (new i = 0;; ++i)
{
printf("[%d]\t%d\t%08x\t%c", i, str1[i], str1[i], str1[i]);
if (str1[i] == '\0')
break;
}
for (new i = 0;; ++i)
{
printf("[%d]\t%d\t%08x\t%c", i, str2[i], str2[i], str2[i]);
if (str2[i] == '\0')
break;
}
}

Здесь в один массив строка копируется прямиком из строковой константы, а в другой она копируется с промежуточным сохранением в SVar.
После этого содержимое массивов выводится сначала полностью, а затем и посимвольно (с номерами позиций в строке и кодами символов).
Вывод:


AaBbZzАаБбЯя
AaBbZzАаБбЯя
[0] 65 00000041 A
[1] 97 00000061 a
[2] 66 00000042 B
[3] 98 00000062 b
[4] 90 0000005A Z
[5] 122 0000007A z
[6] 192 000000C0 А
[7] 224 000000E0 а
[8] 193 000000C1 Б
[9] 225 000000E1 б
[10] 223 000000DF Я
[11] 255 000000FF я
[12] 0 00000000
[0] 65 00000041 A
[1] 97 00000061 a
[2] 66 00000042 B
[3] 98 00000062 b
[4] 90 0000005A Z
[5] 122 0000007A z
[6] -64 FFFFFFC0 А
[7] -32 FFFFFFE0 а
[8] -63 FFFFFFC1 Б
[9] -31 FFFFFFE1 б
[10] -33 FFFFFFDF Я
[11] -1 FFFFFFFF я
[12] 0 00000000

Как и говорил Untonyst, второй массив после сохранения в SVar'е с помощью print и printf выводится нормально, но посмотрите внимательно на коды его последних символов:


[4] 90 0000005A Z
[5] 122 0000007A z
[6] -64 FFFFFFC0 А
[7] -32 FFFFFFE0 а
[8] -63 FFFFFFC1 Б
[9] -31 FFFFFFE1 б
[10] -33 FFFFFFDF Я
[11] -1 FFFFFFFF я
[12] 0 00000000

Ага, у русских символов старшие байты ячеек установлены в FF (255) вместо 00 !
Неудивительно, что они некорректно сохранялись в файл.

Опытным путём удалось выяснить, что все символы с кодом от 0 до 127 обрабатываются корректно.
Сам баг проявляется только в символах с кодами от 128 до 255 - среди них как раз и находятся символы кириллицы.
Скорее всего, Kalcor перед возвратом строки в GetPVarString конвертировал символы из char в cell (т.е. с расширением знакового бита, т.к. оба типа данных не беззнаковые), из-за чего коды символов больше 127 после получения из PVar/SVar'а становились неправильными.

Чтобы исправить символ, достаточно лишь установить старшие байты ячейки в 0, при этом оставив младший байт, как есть.
Это можно легко сделать, выполнив побитовое "И" кода символа с числом 0x000000FF, т.к. будут действовать следующие правила:

X & 0xFF = X
X & 0x00 = 0

В итоге получаем функцию:


FixSVarString(str[], size = sizeof(str))
for (new i = 0; ((str[i] &= 0xFF) != '\0') && (++i != size);) {}

(Ещё вместо побитового "И" можно было вычислить остаток от деления на 256, но для процессора эта операция куда более дорогостоящая.)

Проверка. В отрывок кода, приведённый выше, ставим вызов получившейся функции:


SetPVarString(playerid, "ban_reason", "проверка");
new reason[128];
GetPVarString(playerid, "ban_reason", reason, sizeof(reason));
DeletePVar(playerid, "ban_reason");
FixSVarString(reason);
BanEx(playerid, reason);

Вывод:


AaBbZzАаБбЯя
AaBbZzАаБбЯя
[0] 65 00000041 A
[1] 97 00000061 a
[2] 66 00000042 B
[3] 98 00000062 b
[4] 90 0000005A Z
[5] 122 0000007A z
[6] 192 000000C0 А
[7] 224 000000E0 а
[8] 193 000000C1 Б
[9] 225 000000E1 б
[10] 223 000000DF Я
[11] 255 000000FF я
[12] 0 00000000
[0] 65 00000041 A
[1] 97 00000061 a
[2] 66 00000042 B
[3] 98 00000062 b
[4] 90 0000005A Z
[5] 122 0000007A z
[6] 192 000000C0 А
[7] 224 000000E0 а
[8] 193 000000C1 Б
[9] 225 000000E1 б
[10] 223 000000DF Я
[11] 255 000000FF я
[12] 0 00000000

Всё работает так, как и должно.

Примерно то же самое я сделал в инклуде dc_kickfix.inc.
Для проверки в тестовом моде снова ввёл команду /ban. В игре отличий никаких, но другое дело в samp.ban:
http://ihost.pro-pawn.ru/image.php?di=EZ7I
Символы кириллицы записались так, как и должны были. Проблема решена.


P.S.: Инклуд dc_kickfix.inc (http://pro-pawn.ru/showthread.php?12837-dc_kickfix-inc) обновлён, баг с записью символов кириллицы исправлен.

UPD: Инклуд fix_K2BEx.inc (http://pro-pawn.ru/showthread.php?12832-fix_K2BEx-inc) тоже дождался обновления.

UPD[2]: Фикс принят в fixes.inc (вернее, в форк fixes.inc от ziggi - насколько я понял, он единственный, кто сейчас работает над обновлением инклуда):

https://github.com/ziggi/sa-mp-fixes

UPD (10.05.18): Как удалось выяснить, причина бага кроется во всё той же самопальной функции set_amxstring, уже успевшей отметиться в баге с выходом за пределы массива (http://pro-pawn.ru/showthread.php?16186) - видимо, создатели SA-MP решили использовать эту кривую самоделку, не осилив стандартную функцию amx_SetString.

int set_amxstring(AMX* amx, cell amx_addr, const char* source, int max)
{
cell* dest = (cell *)(amx->base + (int)(((AMX_HEADER *)amx->base)->dat + amx_addr));
cell* start = dest;
while (max--&&*source)
*dest++=(cell)*source++;
*dest = 0;
return dest-start;
}

Обратите внимание на эту строку:

*dest++=(cell)*source++;

Как и предполагалось, при записи символа в массив Pawn происходит конверсия из char в cell, т.е. с расширением знакового бита. Значения больше 127 воспринимаются как отрицательные, например 128 => -128 (0x80), 129 => -127 (0x81), 130 => -126 (0x82), ..., 255 => -1 (0xFF), соответственно при конверсии в cell эти значения всё так же получаются отрицательными (0x80 => 0xFFFFFF80, 0x81 => 0xFFFFFF81, ..., 0xFF => 0xFFFFFFFF), со старшими байтами равными FF - отсюда и баг.
Чтобы устранить эту проблему, достаточно указанную выше строку заменить на:

*dest++=(cell)(unsigned char)*source++;

После этого конверсия будет происходить из беззнакового типа (без расширения знакового бита) и баг будет устранён.


Автор: Daniel_Cortez (http://pro-pawn.ru/member.php?100-Daniel_Cortez)


Специально для Pro-Pawn.ru (http://www.pro-pawn.ru)
Копирование данной статьи на других ресурсах без разрешения автора запрещено!

Nexius_Tailer
15.01.2016, 23:25
Полезная информация, спасибо за решение.
Может, стоит написать об этом на официальный форум? (в раздел багов)

$continue$
15.01.2016, 23:39
Полезная информация, спасибо за решение.
Может, стоит написать об этом на официальный форум? (в раздел багов)

Зачем мне это фиксить? Это же не ошибка безопасности. Да и есть возможность зафиксить скриптово (С) Kyeman

Nexius_Tailer
15.01.2016, 23:58
Зачем мне это фиксить? Это же не ошибка безопасности. Да и есть возможность зафиксить скриптово (С) Kyeman
Отрепортить всё-же лучше, чем думать за него.

ziggi
16.01.2016, 00:15
Отрепортить всё-же лучше, чем думать за него.

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


stock FixAscii(text[])
{
for (new i = 0; text[i] != '\0'; i++) {
text[i] &= 0xFF;
}
}

Nexius_Tailer
16.01.2016, 00:26
Не исправят, не надейся. Существуют целые библиотеки, которые исправляют баги сампа.
По поводу функции: не понимаю, зачем так усложнять код?


stock FixAscii(text[])
{
for (new i = 0; text[i] != '\0'; i++) {
text[i] &= 0xFF;
}
}

Если конкретно это уже есть в fixes.inc или ещё где, то и не надеялся бы)
Думал, обнаружили недавно

Хотя с другой стороны, мелкие баги медленно, но всё-же исправляются (просто об этом в релизе не упоминается)

ziggi
16.01.2016, 01:15
Если конкретно это уже есть в fixes.inc или ещё где, то и не надеялся бы)
Думал, обнаружили недавно

Хотя с другой стороны, мелкие баги медленно, но всё-же исправляются (просто об этом в релизе не упоминается)

Добавил в свой форк fixes.inc (выключено по умолчанию): https://github.com/ziggi/sa-mp-fixes

Daniel_Cortez
16.01.2016, 05:39
Полезная информация, спасибо за решение.
Может, стоит написать об этом на официальный форум? (в раздел багов)
Кому написать? Калькору, что ли?..



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


stock FixAscii(text[])
{
for (new i = 0; text[i] != '\0'; i++) {
text[i] &= 0xFF;
}
}

Производительность. Чем она больше, тем лучше, т.к. от фикса должно быть как можно меньше дополнительной нагрузки.
К тому же, функция не такая уж и сложная сама по себе, чтобы запутаться в коде.

ziggi, в твоём варианте "text[i] != '\0'" и "text[i] &= 0xFF" компилируются в два отдельных предложения, а компилятор оптимизирует код только в пределах одного предложения. Именно поэтому и приходится многие оптимизации делать вручную - ладно бы один раз происходило лишнее действие, но в цикле...
Вообще из-за for получается пара лишних джампов, поэтому я бы порекомендовал вариант с циклом do-while:


static i; i = -1;
do {} while((text[++i] &= 0xFF) != '\0');

Код получится довольно простой, поэтому есть смысл подставить его прямиком в функции с фиксами для Get(P/S)VarString, а не выносить в отдельную функцию.

P.S.: Описание бага в fixes.inc тоже неправильное. Багу подвержены не только символы кириллицы, а вообще все символы, код которых >= 128.

ziggi
16.01.2016, 06:06
ziggi, в твоём варианте "text[i] != '\0'" и "text[i] &= 0xFF" компилируются в два отдельных предложения, а компилятор оптимизирует код только в пределах одного предложения. Именно поэтому и приходится многие оптимизации делать вручную - ладно бы один раз происходило лишнее действие, но в цикле...
Вообще из-за for получается пара лишних джампов, поэтому я бы порекомендовал вариант с циклом do-while:


do {} while((text[i] &= 0xFF) != '\0');

Код получится довольно простой, поэтому есть смысл подставить его прямиком в функции с фиксами для Get(P/S)VarString, а не выносить в отдельную функцию.

P.S.: Описание бага в fixes.inc тоже неправильное. Багу подвержены не только символы кириллицы, а вообще все символы, код которых >= 128.

Спасибо, исправил.
Просто я руководствуюсь простым правилом: если один код нужно выполнить больше, чем в одном месте, то его следует вынести в функцию. Но в данном случае согласен, скорость важнее.

Daniel_Cortez
16.01.2016, 06:12
Спасибо, исправил.
Просто я руководствуюсь простым правилом: если один код нужно выполнить больше, чем в одном месте, то его следует вынести в функцию. Но в данном случае согласен, скорость важнее.
Если код достаточно простой, есть смысл инлайнить его. Многие компиляторы C/C++ обычно так и делают.
Btw, я только что исправил свой пост выше, в примере не было инкремента переменной i и сама переменная не была объявлена (немного поспешил, у нас тут пары сейчас - я записываю конспекты на планшете быстрее, чем остальные в тетради, поэтому остаётся немного свободного времени). Замечу, здесь лучше использовать static, т.к. с new компилятору придётся вставить лишние опкоды для создания локальной переменной в стеке и высвобождения места. Лучше потратить 4 байта в секции данных, чем 16 в секции кода (4 на опкод push, 4 на операнд опкода, 4 на опкод stack и ещё 4 на операнд).

Salvacore
16.01.2016, 07:47
Одмееен, пикчи не грузятся

Daniel_Cortez
16.01.2016, 08:59
Одмееен, пикчи не грузятся
Уже успел заметить. Как приду домой - перезалью.

UPD: Исправил картинки в теме.

UPD[2]: Добавил PR (https://github.com/ziggi/sa-mp-fixes/pull/6) (pull request) в репозиторий форка fixes.inc.

Nash_Brigers
30.03.2016, 12:09
Мужики, а не проще вот так делать?

SetPVarString(playerid, "test_pvar_string", "Тестовый текст.");

new t_str[20];
GetPVarString(playerid, "test_pvar_string", t_str, sizeof(t_str));
format(t_str, 20, "%s", t_str);
SendClientMessage(playerid, 0xFFFFFFFF, t_str);В конечном итоге в t_str записано всё чётко, без всякиях яяяяяпряяя..

Daniel_Cortez
30.03.2016, 14:24
Мужики, а не проще вот так делать?

SetPVarString(playerid, "test_pvar_string", "Тестовый текст.");

new t_str[20];
GetPVarString(playerid, "test_pvar_string", t_str, sizeof(t_str));
format(t_str, 20, "%s", t_str);
SendClientMessage(playerid, 0xFFFFFFFF, t_str);В конечном итоге в t_str записано всё чётко, без всякиях яяяяяпряяя..
По сути в format тот же баг, что и в printf, я описал его в 1-м посте: эти функции учитывают только 0-й байт в ячейке (по той же причине они не способны правильно работать с упакованными строками). Здесь же получается, что один баг может помочь обойти другой.
Тем не менее, полагаться на баги (тем более, использовать один баг, чтобы избежать другого) - отнюдь не самая лучшая практика, т.к. этот баг со временем могут исправить. Я знаю, что в SA:MP не стоит ждать багфиксов, но в любом другом случае вас бы закидали помидорами (или чем-нибудь похуже).
И на счёт простоты неплохо бы сделать тест и проверить производительность format в сравнении с предложенным мной циклом.

Nash_Brigers
31.03.2016, 12:39
но в любом другом случае вас бы закидали помидорами (или чем-нибудь похуже).Золотом?)


И на счёт простоты неплохо бы сделать тест и проверить производительность format в сравнении с предложенным мной циклом.Проверил на миллионном цикле с JIT'ом.. При небольшом тексте Ваш метод выигрывает почти в два раза, при 144 символов уже одинаково с format'ом, при 288 символов - format уже почти на треть быстрее и т.д.
p.s. подозреваю, что в формате не случайно этот "баг"...

Daniel_Cortez
09.05.2018, 23:48
Небольшое обновление:

Как удалось выяснить, причина бага кроется во всё той же самопальной функции set_amxstring, уже успевшей отметиться в баге с выходом за пределы массива (http://pro-pawn.ru/showthread.php?16186) - видимо, создатели SA-MP решили использовать эту кривую самоделку, не осилив стандартную функцию amx_SetString.



int set_amxstring(AMX* amx, cell amx_addr, const char* source, int max)
{
cell* dest = (cell *)(amx->base + (int)(((AMX_HEADER *)amx->base)->dat + amx_addr));
cell* start = dest;
while (max--&&*source)
*dest++=(cell)*source++;
*dest = 0;
return dest-start;
}

Обратите внимание на эту строку:

*dest++=(cell)*source++;

Как и предполагалось, при записи символа в массив Pawn происходит конверсия из char в cell, т.е. с расширением знакового бита. Значения больше 127 воспринимаются как отрицательные, например 128 - -128 (0x80), 129 - -127 (0x81), 130 - -126 (0x82), ..., 255 - -1 (0xFF), соответственно при конверсии в cell эти значения всё так же получаются отрицательными (0x80 => 0xFFFFFF80, 0x81 => 0xFFFFFF81, ..., 0xFF => 0xFFFFFFFF), со старшими байтами равными FF - отсюда и баг.
Чтобы устранить эту проблему, достаточно указанную выше строку заменить на:

*dest++=(cell)(unsigned char)*source++;

После этого конверсия будет происходить из беззнакового типа (без расширения знакового бита) и баг будет устранён.


P.S.: О первопричине бага и способе исправления я уже сообщил 0x452, баг исправлен в предстоящем релизе RW-MP.