From 15398cbb17010fcc7339ce4c0735127e376fc654 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 4 Jun 2026 20:02:24 +0000 Subject: [PATCH 1/2] network: harden tool default_param against an empty param from a client A network client controls nwc_tool_t::default_param. A zero-length wire string deserialises to a NULL plainstring (rdwr_str leaves the buffer NULL for len==0), yet the single-player tool handlers assume the non-NULL param the GUI supplies -- so an empty param reaches atoi(NULL)/*p derefs and the default_param[1] rotation reads past an empty buffer, crashing a server. Normalise NULL to "" at both origins that feed the tool subsystem: the nwc_tool_t::rdwr wire-loading path, and the local nwc_tool_t constructor (a non-network-safe tool used during sync_step is queued via command_queue_append and rebuilt through init_tool() with no wire trip). NULL and "" must then behave the same, so convert the tool sites that branched on the pointer -- ==NULL / !=NULL / the default_param[1] rotation derefs -- to strempty(). tool_change_line_t additionally walked one byte past the terminator on an empty/all-whitespace param; bail out early. --- src/simutrans/network/network_cmd_ingame.cc | 13 +++++++++++ src/simutrans/tool/simtool.cc | 24 +++++++++++++-------- src/simutrans/tool/simtool.h | 3 ++- 3 files changed, 30 insertions(+), 10 deletions(-) diff --git a/src/simutrans/network/network_cmd_ingame.cc b/src/simutrans/network/network_cmd_ingame.cc index 044a0da62..ec0aafeef 100644 --- a/src/simutrans/network/network_cmd_ingame.cc +++ b/src/simutrans/network/network_cmd_ingame.cc @@ -1063,6 +1063,12 @@ nwc_tool_t::nwc_tool_t(player_t *player, tool_t *tool_, koord3d pos_, uint32 syn tool_id = tool_->get_id(); wt = tool_->get_waytype(); default_param = tool_->get_default_param(player); + if( default_param == NULL ) { + // Same NULL -> "" guard as rdwr below, for the local origin: a + // non-network-safe tool queued during sync_step (call_work_api -> + // command_queue_append) is rebuilt via init_tool() with no wire trip. + default_param = ""; + } init = init_; tool_client_id = 0; flags = tool_->flags; @@ -1117,6 +1123,13 @@ void nwc_tool_t::rdwr() packet->rdwr_short(tool_id); packet->rdwr_short(wt); packet->rdwr_str(default_param); + if( packet->is_loading() && default_param == NULL ) { + // rdwr_str yields a NULL plainstring for a len==0 wire string, and the + // wire can't tell an empty param from "no param" anyway. Normalise to "" + // so tools never see a NULL here; they treat "" and NULL identically + // (the strempty() guards in simtool). + default_param = ""; + } packet->rdwr_bool(init); packet->rdwr_long(tool_client_id); packet->rdwr_byte(flags); diff --git a/src/simutrans/tool/simtool.cc b/src/simutrans/tool/simtool.cc index 16db9eaca..6cda86774 100644 --- a/src/simutrans/tool/simtool.cc +++ b/src/simutrans/tool/simtool.cc @@ -3493,7 +3493,7 @@ image_id tool_wayremover_t::get_icon(player_t *) const waytype_t tool_wayremover_t::get_waytype() const { - return default_param ? (waytype_t)atoi(default_param) : invalid_wt; + return !strempty(default_param) ? (waytype_t)atoi(default_param) : invalid_wt; } class electron_t : public test_driver_t { @@ -5834,7 +5834,7 @@ image_id tool_build_depot_t::get_icon(player_t *player) const bool tool_build_depot_t::init( player_t *player ) { - if (default_param == NULL) { + if (strempty(default_param)) { return false; } @@ -6072,7 +6072,7 @@ const char *tool_build_land_chain_t::work( player_t *player, koord3d pos ) if(fab==NULL) { return ""; } - int rotation = (default_param && default_param[1]!='#') ? (default_param[1]-'0') % fab->get_building()->get_all_layouts() : simrand(fab->get_building()->get_all_layouts()-1); + int rotation = (!strempty(default_param) && default_param[1]!='#') ? (default_param[1]-'0') % fab->get_building()->get_all_layouts() : simrand(fab->get_building()->get_all_layouts()-1); koord size = fab->get_building()->get_size(rotation); // process ignore climates switch @@ -6084,7 +6084,7 @@ const char *tool_build_land_chain_t::work( player_t *player, koord3d pos ) // at sea hat_platz = welt->is_water( pos.get_2d(), fab->get_building()->get_size(rotation) ); - if(!hat_platz && size.y!=size.x && fab->get_building()->get_all_layouts()>1 && (default_param==NULL || default_param[1]=='#')) { + if(!hat_platz && size.y!=size.x && fab->get_building()->get_all_layouts()>1 && (strempty(default_param) || default_param[1]=='#')) { // try other rotation too ... rotation = (rotation+1) % fab->get_building()->get_all_layouts(); hat_platz = welt->is_water( pos.get_2d(), fab->get_building()->get_size(rotation) ); @@ -6094,7 +6094,7 @@ const char *tool_build_land_chain_t::work( player_t *player, koord3d pos ) // and on solid ground hat_platz = welt->square_is_free( pos.get_2d(), fab->get_building()->get_x(rotation), fab->get_building()->get_y(rotation), NULL, cl ); - if(!hat_platz && size.y!=size.x && fab->get_building()->get_all_layouts()>1 && (default_param==NULL || default_param[1]=='#')) { + if(!hat_platz && size.y!=size.x && fab->get_building()->get_all_layouts()>1 && (strempty(default_param) || default_param[1]=='#')) { // try other rotation too ... rotation = (rotation+1) % fab->get_building()->get_all_layouts(); hat_platz = welt->square_is_free( pos.get_2d(), fab->get_building()->get_x(rotation), fab->get_building()->get_y(rotation), NULL, cl ); @@ -6267,14 +6267,14 @@ const char *tool_build_factory_t::work( player_t *player, koord3d pos ) return ""; } - int rotation = (default_param && default_param[1]!='#') ? (default_param[1]-'0') % fab->get_building()->get_all_layouts() : simrand(fab->get_building()->get_all_layouts()); + int rotation = (!strempty(default_param) && default_param[1]!='#') ? (default_param[1]-'0') % fab->get_building()->get_all_layouts() : simrand(fab->get_building()->get_all_layouts()); // process ignore climates switch climate_bits cl = (default_param && default_param[0]=='1') ? ALL_CLIMATES : fab->get_building()->get_allowed_climate_bits(); bool hat_platz = this->can_build_factory_here(fab, pos.get_2d(), rotation, cl); - if (!hat_platz && fab->get_building()->get_all_layouts()>1 && (default_param==NULL || default_param[1]=='#')) { + if (!hat_platz && fab->get_building()->get_all_layouts()>1 && (strempty(default_param) || default_param[1]=='#')) { // try other rotation rotation = (rotation +1) % fab->get_building()->get_all_layouts(); hat_platz = this->can_build_factory_here(fab, pos.get_2d(), rotation, cl); @@ -7974,9 +7974,15 @@ bool tool_change_line_t::init( player_t *player ) // skip the rest of the command const char *p = default_param; - while( *p && *p<=' ' ) { + while( !strempty(p) && *p<=' ' ) { p++; } + if( strempty(p) ) { + // empty or all-whitespace param: nothing to parse, and bailing here + // keeps the *p++ below from reading past the terminator + dbg->warning( "tool_change_line_t::init()", "too short command \"%s\"", default_param ); + return false; + } char tool=*p++; while( *p && *p++!=',' ) { @@ -8433,7 +8439,7 @@ bool tool_change_depot_t::init( player_t *player ) */ bool tool_change_player_t::init( player_t *player_in) { - if( default_param==NULL ) { + if( strempty(default_param) ) { dbg->warning( "tool_change_player_t::init()", "nothing to do!" ); return false; } diff --git a/src/simutrans/tool/simtool.h b/src/simutrans/tool/simtool.h index 4c040c1ba..8c91bd212 100644 --- a/src/simutrans/tool/simtool.h +++ b/src/simutrans/tool/simtool.h @@ -22,6 +22,7 @@ #include "../dataobj/environment.h" #include "../dataobj/translator.h" +#include "../utils/simstring.h" #include "../display/viewport.h" @@ -145,7 +146,7 @@ class tool_setslope_t : public tool_t { bool is_init_keeps_game_state() const OVERRIDE { return true; } char const* check_pos(player_t*, koord3d) OVERRIDE; char const* work(player_t* const player, koord3d const k) OVERRIDE { return tool_set_slope_work(player, k, default_param ? atoi(default_param) : 0, old_slope_compatibility_mode); } - bool init(player_t*) OVERRIDE { return default_param != NULL; } + bool init(player_t*) OVERRIDE { return !strempty(default_param); } }; class tool_restoreslope_t : public tool_t { From 08470c3e560e610ae33dd476d6fcd3f35f5c3735 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 4 Jun 2026 20:02:52 +0000 Subject: [PATCH 2/2] tool: treat an empty build_house param like no param (random attraction) build_house rejected "" as invalid (init returned false) while treating NULL as "build a random attraction". With empty tool params now normalised to "" on the command paths -- and the wire unable to tell "" from "no param" in any case -- fold "" into the random-attraction path so a no-param request behaves the same however it arrived. The test that asserted "" -> error now expects the random-attraction result (no suitable ground at the test coordinate). --- src/simutrans/tool/simtool.cc | 6 +++--- tests/tests/test_building.nut | 13 +++---------- 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/src/simutrans/tool/simtool.cc b/src/simutrans/tool/simtool.cc index 6cda86774..0d927796c 100644 --- a/src/simutrans/tool/simtool.cc +++ b/src/simutrans/tool/simtool.cc @@ -5931,7 +5931,7 @@ const char *tool_build_depot_t::work( player_t *player, koord3d pos ) */ bool tool_build_house_t::init( player_t * ) { - if (default_param && strlen(default_param) < 3) { + if (!strempty(default_param) && strlen(default_param) < 3) { return false; } @@ -5973,7 +5973,7 @@ const char *tool_build_house_t::work( player_t *player, koord3d pos ) return ""; } int rotation; - if( !default_param || default_param[1]=='#' ) { + if( strempty(default_param) || default_param[1]=='#' ) { rotation = simrand(desc->get_all_layouts()); } else if( default_param[1]=='A' ) { @@ -6002,7 +6002,7 @@ const char *tool_build_house_t::work( player_t *player, koord3d pos ) climate_bits cl = (default_param && default_param[0]=='1') ? ALL_CLIMATES : desc->get_allowed_climate_bits(); bool hat_platz = welt->square_is_free( k, desc->get_x(rotation), desc->get_y(rotation), NULL, cl ); - if(!hat_platz && size.y!=size.x && desc->get_all_layouts()>1 && (default_param==NULL || default_param[1]=='#' || default_param[1]=='A')) { + if(!hat_platz && size.y!=size.x && desc->get_all_layouts()>1 && (strempty(default_param) || default_param[1]=='#' || default_param[1]=='A')) { // try other rotation too ... rotation = (rotation+1) % desc->get_all_layouts(); hat_platz = welt->square_is_free( k, desc->get_x(rotation), desc->get_y(rotation), NULL, cl ); diff --git a/tests/tests/test_building.nut b/tests/tests/test_building.nut index 32c8a915e..35ebdde4c 100644 --- a/tests/tests/test_building.nut +++ b/tests/tests/test_building.nut @@ -17,17 +17,10 @@ function test_building_build_house_invalid_param() ASSERT_EQUAL(command_x(tool_build_house).work(pl, coord3d(-1, 1, 0)), "") } - // empty default_param: error + // empty default_param: now treated like no param (random attraction). + // (0,0,0) has no suitable ground, so it reports that rather than erroring. { - local error_caught = false - try { - ASSERT_EQUAL(command_x(tool_build_house).work(pl, coord3d(0, 0, 0), ""), null) - } - catch (e) { - error_caught = true - ASSERT_EQUAL(e, "Error during initializing tool") - } - ASSERT_TRUE(error_caught) + ASSERT_EQUAL(command_x(tool_build_house).work(pl, coord3d(0, 0, 0), ""), "No suitable ground!") } // Invalid default_param: error