From b826bc2c54dbe6a2ad0f204f36f82bf116d8e663 Mon Sep 17 00:00:00 2001 From: ultramage Date: Sat, 22 Sep 2007 11:02:26 +0000 Subject: * Added 'safestrnlen' to prevent null pointer crashes * Fixed global chat logging always crashing on a null pointer * Applied changes to clif_parse_globalmessage() from my WiP code - clearer processing of the individual packet components - proper code ordering, some more integrity checks - fixes to some poorly chosen ShowWarning() format strings - global chat logging no longer logs the entire string (w/ player name) git-svn-id: https://rathena.svn.sourceforge.net/svnroot/rathena/trunk@11271 54d463be-8e91-2dee-dedb-b68131a5f0ec --- Changelog-Trunk.txt | 7 +++ src/char_sql/int_guild.c | 2 +- src/char_sql/int_party.c | 2 +- src/common/mapindex.c | 2 +- src/common/strlib.c | 6 +++ src/common/strlib.h | 3 ++ src/map/clif.c | 111 +++++++++++++++++++++++++++-------------------- src/map/log.c | 8 ++-- src/map/script.c | 2 +- 9 files changed, 87 insertions(+), 56 deletions(-) diff --git a/Changelog-Trunk.txt b/Changelog-Trunk.txt index 1062d1481..c8a9cd44d 100644 --- a/Changelog-Trunk.txt +++ b/Changelog-Trunk.txt @@ -7,6 +7,13 @@ IF YOU HAVE A WORKING AND TESTED BUGFIX PUT IT INTO STABLE AS WELL AS TRUNK. * Added a sanity check for MAX_ZENY (doesn't compile if too big). * Redid the buildin_query_sql function. (fixes bugreport:81). [FlavioJS] 2007/09/21 + * Applied changes to clif_parse_globalmessage() from my WiP code + - clearer processing of the individual packet components + - proper code ordering, some more integrity checks + - fixes to some poorly chosen ShowWarning() format strings + - global chat logging no longer logs the entire string (w/ player name) + * Fixed global chat logging always crashing on a null pointer + * Added 'safestrnlen' to prevent null pointer crashes [ultramage] * itemdb.c/h using a static array of 32k struct item_data* entries (faster itemdb loockup and a first step to remove map_session_data->inventory_data). * Fixed a typo in the configure script that replaced CFLAGS with CPPFLAGS diff --git a/src/char_sql/int_guild.c b/src/char_sql/int_guild.c index f33f58097..663cb4b6b 100644 --- a/src/char_sql/int_guild.c +++ b/src/char_sql/int_guild.c @@ -829,7 +829,7 @@ int search_guildname(char *str) int guild_id; char esc_name[NAME_LENGTH*2+1]; - Sql_EscapeStringLen(sql_handle, esc_name, str, strnlen(str, NAME_LENGTH)); + Sql_EscapeStringLen(sql_handle, esc_name, str, safestrnlen(str, NAME_LENGTH)); //Lookup guilds with the same name if( SQL_ERROR == Sql_Query(sql_handle, "SELECT guild_id FROM `%s` WHERE name='%s'", guild_db, esc_name) ) { diff --git a/src/char_sql/int_party.c b/src/char_sql/int_party.c index f6d783b3b..fd1f0c37c 100644 --- a/src/char_sql/int_party.c +++ b/src/char_sql/int_party.c @@ -299,7 +299,7 @@ struct party_data* search_partyname(char* str) char* data; struct party_data* p = NULL; - Sql_EscapeStringLen(sql_handle, esc_name, str, strnlen(str, NAME_LENGTH)); + Sql_EscapeStringLen(sql_handle, esc_name, str, safestrnlen(str, NAME_LENGTH)); if( SQL_ERROR == Sql_Query(sql_handle, "SELECT `party_id` FROM `%s` WHERE `name`='%s'", party_db, esc_name) ) Sql_ShowDebug(sql_handle); else if( SQL_SUCCESS == Sql_NextRow(sql_handle) ) diff --git a/src/common/mapindex.c b/src/common/mapindex.c index f2adc833c..2fa8c9fb8 100644 --- a/src/common/mapindex.c +++ b/src/common/mapindex.c @@ -52,7 +52,7 @@ const char* mapindex_getmapname_ext(const char* string, char* output) static char buf[MAP_NAME_LENGTH_EXT]; char* dest = (output != NULL) ? output : buf; - size_t len = strnlen(string, MAP_NAME_LENGTH); + size_t len = safestrnlen(string, MAP_NAME_LENGTH); if (len == MAP_NAME_LENGTH) { ShowWarning("(mapindex_normalize_name) Map name '%*s' is too long!", 2*MAP_NAME_LENGTH, string); len--; diff --git a/src/common/strlib.c b/src/common/strlib.c index 4f204a768..c0a6238f9 100644 --- a/src/common/strlib.c +++ b/src/common/strlib.c @@ -310,6 +310,12 @@ char* safestrncpy(char* dst, const char* src, size_t n) return ret; } +/// doesn't crash on null pointer +size_t safestrnlen(const char* string, size_t maxlen) +{ + return ( string != NULL ) ? strnlen(string, maxlen) : 0; +} + ///////////////////////////////////////////////////////////////////// // StringBuf - dynamic string diff --git a/src/common/strlib.h b/src/common/strlib.h index ee473f466..fd24b4b24 100644 --- a/src/common/strlib.h +++ b/src/common/strlib.h @@ -34,6 +34,9 @@ int config_switch(const char* str); /// always nul-terminates the string char* safestrncpy(char* dst, const char* src, size_t n); +/// doesn't crash on null pointer +size_t safestrnlen(const char* string, size_t maxlen); + /// StringBuf - dynamic string struct StringBuf { diff --git a/src/map/clif.c b/src/map/clif.c index fab1e00ad..5c3b62f23 100644 --- a/src/map/clif.c +++ b/src/map/clif.c @@ -8384,66 +8384,80 @@ void clif_parse_GetCharNameRequest(int fd, struct map_session_data *sd) /*========================================== * Validates and processes global messages - * S 008c/00f3 .w .?B + * S 008c/00f3 .w .?B ( : ) *------------------------------------------*/ void clif_parse_GlobalMessage(int fd, struct map_session_data* sd) { - char* message; - unsigned int packetlen, messagelen, namelen; + const char *text, *name, *message; + unsigned int packetlen, textlen, namelen, messagelen; packetlen = RFIFOW(fd,2); - if (packetlen > RFIFOREST(fd)) { // there has to be enough data to read + // basic structure checks + if( packetlen > RFIFOREST(fd) ) + { // there has to be enough data to read ShowWarning("clif_parse_GlobalMessage: Received malformed packet from player '%s' (length is incorrect)!", sd->status.name); return; } - if (packetlen < 4 + 1) { // 4-byte header and at least an empty string is expected + if( packetlen < 4 + 1 ) + { // 4-byte header and at least an empty string is expected ShowWarning("clif_parse_GlobalMessage: Received malformed packet from player '%s' (no message data)!", sd->status.name); return; } - message = (char*)RFIFOP(fd,4); - messagelen = packetlen - 4; // let's trust the client here, nothing can go wrong and it saves us one strlen() - if (messagelen > CHAT_SIZE) { // messages mustn't be too long + text = (char*)RFIFOP(fd,4); + textlen = packetlen - 4; + + name = text; + namelen = strnlen(sd->status.name, NAME_LENGTH - 1); + // verify part of the packet + if( strncmp(name, sd->status.name, namelen) || // the text must start with the speaker's name + name[namelen] != ' ' || name[namelen+1] != ':' || name[namelen+2] != ' ' ) // followed by ' : ' + { + //Hacked message, or infamous "client desynch" issue where they pick one char while loading another. + ShowWarning("clif_parse_GlobalMessage: Player '%s' sent a message using an incorrect name! Forcing a relog...", sd->status.name); + clif_setwaitclose(fd); // Just kick them out to correct it. + return; + } + + message = text + namelen + 3; + messagelen = textlen - namelen - 3 - 1; // this should be the message length + // verify part of the packet + if( message[messagelen] != '\0' ) + { // message must be zero-terminated + ShowWarning("clif_parse_GlobalMessage: Player '%s' sent an unterminated string!", sd->status.name); + return; + } + if( messagelen != strnlen(message, messagelen+1) ) + { // the declared length must match real length + ShowWarning("clif_parse_GlobalMessage: Received malformed packet from player '%s' (length is incorrect)!", sd->status.name); + return; + } + if( messagelen > CHATBOX_SIZE ) + { // messages mustn't be too long int i; // special case here - allow some more freedom for frost joke & dazzler // TODO:? You could use a state flag when FrostJoke/Scream is used, and unset it once the skill triggers. [Skotlex] - for(i = 0; i < MAX_SKILLTIMERSKILL && sd->ud.skilltimerskill[i] && - sd->ud.skilltimerskill[i]->skill_id != BA_FROSTJOKE && - sd->ud.skilltimerskill[i]->skill_id != DC_SCREAM; i++); + ARR_FIND( 0, MAX_SKILLTIMERSKILL, i, sd->ud.skilltimerskill[i] == 0 || sd->ud.skilltimerskill[i]->skill_id == BA_FROSTJOKE || sd->ud.skilltimerskill[i]->skill_id == DC_SCREAM ); - if (i == MAX_SKILLTIMERSKILL || !sd->ud.skilltimerskill[i]) { // normal message, too long - ShowWarning("clif_parse_GlobalMessage: Player '%s' sent a message too long ('%.*s')!", sd->status.name, CHAT_SIZE, message); + if( i == MAX_SKILLTIMERSKILL || !sd->ud.skilltimerskill[i]) + { // normal message, too long + ShowWarning("clif_parse_GlobalMessage: Player '%s' sent a message too long ('%.*s')!", sd->status.name, CHATBOX_SIZE, message); return; } - if (messagelen > 255) { // frost joke/dazzler, but still too long + if( messagelen > 255 ) + { // frost joke/dazzler, but still too long ShowWarning("clif_parse_GlobalMessage: Player '%s' sent a message too long ('%.*s')!", sd->status.name, 255, message); return; } } - if (message[messagelen-1] != '\0') { // message must be zero-terminated - ShowWarning("clif_parse_GlobalMessage: Player '%s' sent an unterminated string!", sd->status.name); - return; - } - namelen = strnlen(sd->status.name, NAME_LENGTH - 1); - if (strncmp(message, sd->status.name, namelen) || // the name has to match the speaker's name - message[namelen] != ' ' || message[namelen+1] != ':' || message[namelen+2] != ' ') // completely, not just the prefix - { - //Hacked message, or infamous "client desynch" issue where they pick one char while loading another. - clif_setwaitclose(fd); // Just kick them out to correct it. - ShowWarning("clif_parse_GlobalMessage: Player '%.*s' sent a message using an incorrect name ('%s')! Forcing a relog...", namelen, sd->status.name, message); - return; - } - - if (is_atcommand(fd, sd, message) != AtCommand_None || is_charcommand(fd, sd, message) != CharCommand_None) + if( is_atcommand(fd, sd, text) != AtCommand_None || is_charcommand(fd, sd, text) != CharCommand_None ) return; - if (sd->sc.count && - (sd->sc.data[SC_BERSERK].timer != -1 || - (sd->sc.data[SC_NOCHAT].timer != -1 && sd->sc.data[SC_NOCHAT].val1&MANNER_NOCHAT))) + if( sd->sc.data[SC_BERSERK].timer != -1 || (sd->sc.data[SC_NOCHAT].timer != -1 && sd->sc.data[SC_NOCHAT].val1&MANNER_NOCHAT) ) return; - if (battle_config.min_chat_delay) + if( battle_config.min_chat_delay ) { //[Skotlex] if (DIFF_TICK(sd->cantalk_tick, gettick()) > 0) return; @@ -8451,11 +8465,11 @@ void clif_parse_GlobalMessage(int fd, struct map_session_data* sd) } // send message to others - WFIFOHEAD(fd, messagelen + 8); + WFIFOHEAD(fd, 8 + textlen); WFIFOW(fd,0) = 0x8d; - WFIFOW(fd,2) = messagelen + 8; + WFIFOW(fd,2) = 8 + textlen; WFIFOL(fd,4) = sd->bl.id; - memcpy(WFIFOP(fd,8), message, messagelen); + safestrncpy((char*)WFIFOP(fd,8), text, textlen); clif_send(WFIFOP(fd,0), WFIFOW(fd,2), &sd->bl, sd->chatID ? CHAT_WOS : AREA_CHAT_WOC); // send back message to the speaker @@ -8465,37 +8479,38 @@ void clif_parse_GlobalMessage(int fd, struct map_session_data* sd) #ifdef PCRE_SUPPORT // trigger listening mobs/npcs - map_foreachinrange(npc_chat_sub, &sd->bl, AREA_SIZE, BL_NPC, message, strlen(message), &sd->bl); - map_foreachinrange(mob_chat_sub, &sd->bl, AREA_SIZE, BL_MOB, message, strlen(message), &sd->bl); + map_foreachinrange(npc_chat_sub, &sd->bl, AREA_SIZE, BL_NPC, text, textlen, &sd->bl); + map_foreachinrange(mob_chat_sub, &sd->bl, AREA_SIZE, BL_MOB, text, textlen, &sd->bl); #endif // check for special supernovice phrase - if ((sd->class_&MAPID_UPPERMASK) == MAPID_SUPER_NOVICE) { + if( (sd->class_&MAPID_UPPERMASK) == MAPID_SUPER_NOVICE ) + { int next = pc_nextbaseexp(sd); - if (next > 0 && (sd->status.base_exp * 1000 / next)% 100 == 0) { // 0%, 10%, 20%, ... + if( next > 0 && (sd->status.base_exp * 1000 / next)% 100 == 0 ) { // 0%, 10%, 20%, ... switch (sd->state.snovice_call_flag) { case 0: - if (strstr(message, msg_txt(504))) // "Guardian Angel, can you hear my voice? ^^;" + if( strstr(message, msg_txt(504)) ) // "Guardian Angel, can you hear my voice? ^^;" sd->state.snovice_call_flag++; break; case 1: { char buf[256]; sprintf(buf, msg_txt(505), sd->status.name); - if (strstr(message, buf)) // "My name is %s, and I'm a Super Novice~" + if( strstr(message, buf) ) // "My name is %s, and I'm a Super Novice~" sd->state.snovice_call_flag++; } break; case 2: - if (strstr(message, msg_txt(506))) // "Please help me~ T.T" + if( strstr(message, msg_txt(506)) ) // "Please help me~ T.T" sd->state.snovice_call_flag++; break; case 3: - if (skillnotok(MO_EXPLOSIONSPIRITS,sd)) - break; //Do not override the noskill mapflag. [Skotlex] - clif_skill_nodamage(&sd->bl,&sd->bl,MO_EXPLOSIONSPIRITS,-1, - sc_start(&sd->bl,SkillStatusChangeTable(MO_EXPLOSIONSPIRITS),100, + if( skillnotok(MO_EXPLOSIONSPIRITS,sd) ) + break; //Do not override the noskill mapflag. [Skotlex] + clif_skill_nodamage(&sd->bl,&sd->bl,MO_EXPLOSIONSPIRITS,-1, + sc_start(&sd->bl,SkillStatusChangeTable(MO_EXPLOSIONSPIRITS),100, 17,skill_get_time(MO_EXPLOSIONSPIRITS,1))); //Lv17-> +50 critical (noted by Poki) [Skotlex] - sd->state.snovice_call_flag = 0; + sd->state.snovice_call_flag = 0; break; } } diff --git a/src/map/log.c b/src/map/log.c index b5c4a3d9c..40225e7b7 100644 --- a/src/map/log.c +++ b/src/map/log.c @@ -298,7 +298,7 @@ int log_atcommand(struct map_session_data* sd, const char* message) stmt = SqlStmt_Malloc(logmysql_handle); if( SQL_SUCCESS != SqlStmt_Prepare(stmt, "INSERT DELAYED INTO `%s` (`atcommand_date`, `account_id`, `char_id`, `char_name`, `map`, `command`) VALUES (NOW(), '%d', '%d', ?, '%s', ?)", log_config.log_gm_db, sd->status.account_id, sd->status.char_id, sd->status.name, mapindex_id2name(sd->mapindex), message) || SQL_SUCCESS != SqlStmt_BindParam(stmt, 0, SQLDT_STRING, sd->status.name, strnlen(sd->status.name, NAME_LENGTH)) - || SQL_SUCCESS != SqlStmt_BindParam(stmt, 1, SQLDT_STRING, (char*)message, strnlen(message, 255)) + || SQL_SUCCESS != SqlStmt_BindParam(stmt, 1, SQLDT_STRING, (char*)message, safestrnlen(message, 255)) || SQL_SUCCESS != SqlStmt_Execute(stmt) ) { SqlStmt_ShowDebug(stmt); @@ -336,7 +336,7 @@ int log_npc(struct map_session_data* sd, const char* message) stmt = SqlStmt_Malloc(logmysql_handle); if( SQL_SUCCESS != SqlStmt_Prepare(stmt, "INSERT DELAYED INTO `%s` (`npc_date`, `account_id`, `char_id`, `char_name`, `map`, `mes`) VALUES (NOW(), '%d', '%d', ?, '%s', ?)", log_config.log_npc_db, sd->status.account_id, sd->status.char_id, sd->status.name, mapindex_id2name(sd->mapindex), message) || SQL_SUCCESS != SqlStmt_BindParam(stmt, 0, SQLDT_STRING, sd->status.name, strnlen(sd->status.name, NAME_LENGTH)) - || SQL_SUCCESS != SqlStmt_BindParam(stmt, 1, SQLDT_STRING, (char*)message, strnlen(message, 255)) + || SQL_SUCCESS != SqlStmt_BindParam(stmt, 1, SQLDT_STRING, (char*)message, safestrnlen(message, 255)) || SQL_SUCCESS != SqlStmt_Execute(stmt) ) { SqlStmt_ShowDebug(stmt); @@ -393,8 +393,8 @@ int log_chat(const char* type, int type_id, int src_charid, int src_accid, const stmt = SqlStmt_Malloc(logmysql_handle); if( SQL_SUCCESS != SqlStmt_Prepare(stmt, "INSERT DELAYED INTO `%s` (`time`, `type`, `type_id`, `src_charid`, `src_accountid`, `src_map`, `src_map_x`, `src_map_y`, `dst_charname`, `message`) VALUES (NOW(), '%s', '%d', '%d', '%d', '%s', '%d', '%d', ?, ?)", log_config.log_chat_db, type, type_id, src_charid, src_accid, map, x, y) - || SQL_SUCCESS != SqlStmt_BindParam(stmt, 0, SQLDT_STRING, (char*)dst_charname, strnlen(dst_charname, NAME_LENGTH)) - || SQL_SUCCESS != SqlStmt_BindParam(stmt, 1, SQLDT_STRING, (char*)message, strnlen(message, CHAT_SIZE)) + || SQL_SUCCESS != SqlStmt_BindParam(stmt, 0, SQLDT_STRING, (char*)dst_charname, safestrnlen(dst_charname, NAME_LENGTH)) + || SQL_SUCCESS != SqlStmt_BindParam(stmt, 1, SQLDT_STRING, (char*)message, safestrnlen(message, CHAT_SIZE)) || SQL_SUCCESS != SqlStmt_Execute(stmt) ) { SqlStmt_ShowDebug(stmt); diff --git a/src/map/script.c b/src/map/script.c index 5dff55cac..37dd99229 100644 --- a/src/map/script.c +++ b/src/map/script.c @@ -3423,7 +3423,7 @@ static int script_save_mapreg_strsub(DBKey key,void *data,va_list ap) #else if ( name[1] != '@') { char tmp_str2[2*255+1]; - Sql_EscapeStringLen(mmysql_handle, tmp_str2, (char*)data, strnlen((char*)data, 255)); + Sql_EscapeStringLen(mmysql_handle, tmp_str2, (char*)data, safestrnlen((char*)data, 255)); if( SQL_ERROR == Sql_Query(mmysql_handle, "UPDATE `%s` SET `%s`='%s' WHERE `%s`='%s' AND `%s`='%d'", mapregsql_db, mapregsql_db_value, tmp_str2, mapregsql_db_varname, name, mapregsql_db_index, i) ) Sql_ShowDebug(mmysql_handle); } -- cgit v1.2.3-70-g09d2