Change: Don't handle 'missing' string parameters as 0. #11673

PeterN · 2024-01-03T03:05:27Z

Motivation / Problem

If not enough parameters are supplied for a string, then a value of 0 was used, which could result in incorrect information being displayed.

Players may not notice there is an issue.

Description

Instead, throw an exception and include an error in the string. This is non-translatable, like existing string error messages.

Limitations

This is not intended as a fix for GSText compatibility, that is another issue :)

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

The bug fix is important enough to be backported? (label: 'backport requested')
This PR touches english.txt or translations? Check the guidelines
This PR affects the save game format? (label 'savegame upgrade')
This PR affects the GS/AI API? (label 'needs review: Script API')
- ai_changelog.hpp, game_changelog.hpp need updating.
- The compatibility wrappers (compat_*.nut) need updating.
This PR affects the NewGRF API? (label 'needs review: NewGRF')
- newgrf_debug_data.h may need updating.
- PR must be added to API tracker

src/strings.cpp

-						case VEH_AIRCRAFT: string_id = STR_SV_AIRCRAFT_NAME; break;
+				case SCC_WAYPOINT_NAME: { // {WAYPOINT}
+					Waypoint *wp = Waypoint::GetIfValid(args.GetNextParameter<StationID>());
+					if (wp == nullptr) break;
+
+					if (!wp->name.empty()) {
+						auto tmp_params = MakeParameters(wp->name);
+						GetStringWithArgs(builder, STR_JUST_RAW_STRING, tmp_params);
+					} else {
+						auto tmp_params = MakeParameters(wp->town->index, wp->town_cn + 1);
+						StringID string_id = ((wp->string_id == STR_SV_STNAME_BUOY) ? STR_FORMAT_BUOY_NAME : STR_FORMAT_WAYPOINT_NAME);
+						if (wp->town_cn != 0) string_id++;
+						GetStringWithArgs(builder, string_id, tmp_params);
 					}
+					break;
+				}

-					GetStringWithArgs(builder, string_id, tmp_params);
+				case SCC_VEHICLE_NAME: { // {VEHICLE}
+					const Vehicle *v = Vehicle::GetIfValid(args.GetNextParameter<VehicleID>());
+					if (v == nullptr) break;
+
+					if (!v->name.empty()) {
+						auto tmp_params = MakeParameters(v->name);
+						GetStringWithArgs(builder, STR_JUST_RAW_STRING, tmp_params);
+					} else if (v->group_id != DEFAULT_GROUP) {
+						/* The vehicle has no name, but is member of a group, so print group name */
+						auto tmp_params = MakeParameters(v->group_id, v->unitnumber);
+						GetStringWithArgs(builder, STR_FORMAT_GROUP_VEHICLE_NAME, tmp_params);
+					} else {
+						auto tmp_params = MakeParameters(v->unitnumber);
+
+						StringID string_id;
+						switch (v->type) {
+							default:           string_id = STR_INVALID_VEHICLE; break;
+							case VEH_TRAIN:    string_id = STR_SV_TRAIN_NAME; break;
+							case VEH_ROAD:     string_id = STR_SV_ROAD_VEHICLE_NAME; break;
+							case VEH_SHIP:     string_id = STR_SV_SHIP_NAME; break;
+							case VEH_AIRCRAFT: string_id = STR_SV_AIRCRAFT_NAME; break;
+						}
+
+						GetStringWithArgs(builder, string_id, tmp_params);
+					}
+					break;
 				}
-				break;
-			}

-			case SCC_SIGN_NAME: { // {SIGN}
-				const Sign *si = Sign::GetIfValid(args.GetNextParameter<SignID>());
-				if (si == nullptr) break;
+				case SCC_SIGN_NAME: { // {SIGN}
+					const Sign *si = Sign::GetIfValid(args.GetNextParameter<SignID>());
+					if (si == nullptr) break;

-				if (!si->name.empty()) {
-					auto tmp_params = MakeParameters(si->name);
-					GetStringWithArgs(builder, STR_JUST_RAW_STRING, tmp_params);
-				} else {
-					auto tmp_params = ArrayStringParameters<0>();
-					GetStringWithArgs(builder, STR_DEFAULT_SIGN_NAME, tmp_params);
+					if (!si->name.empty()) {
+						auto tmp_params = MakeParameters(si->name);
+						GetStringWithArgs(builder, STR_JUST_RAW_STRING, tmp_params);
+					} else {
+						auto tmp_params = ArrayStringParameters<0>();
+						GetStringWithArgs(builder, STR_DEFAULT_SIGN_NAME, tmp_params);
+					}
+					break;
 				}
-				break;
-			}

-			case SCC_STATION_FEATURES: { // {STATIONFEATURES}
-				StationGetSpecialString(builder, args.GetNextParameter<StationFacility>());
-				break;
-			}
+				case SCC_STATION_FEATURES: { // {STATIONFEATURES}
+					StationGetSpecialString(builder, args.GetNextParameter<StationFacility>());
+					break;
+				}

-			case SCC_COLOUR: { // {COLOUR}
-				StringControlCode scc = (StringControlCode)(SCC_BLUE + args.GetNextParameter<Colours>());
-				if (IsInsideMM(scc, SCC_BLUE, SCC_COLOUR)) builder.Utf8Encode(scc);
-				break;
-			}
+				case SCC_COLOUR: { // {COLOUR}
+					StringControlCode scc = (StringControlCode)(SCC_BLUE + args.GetNextParameter<Colours>());
+					if (IsInsideMM(scc, SCC_BLUE, SCC_COLOUR)) builder.Utf8Encode(scc);
+					break;
+				}

-			default:
-				builder.Utf8Encode(b);
-				break;
+				default:
+					builder.Utf8Encode(b);
+					break;
+			}


src/strings.cpp

glx22 · 2024-01-03T21:29:18Z

PeterN/OpenTTD@invalid-parameters...glx22:OpenTTD:pr/11673 tested with

		local text = GSText(GSText.STR_TEST);
		text.SetParam(1, GSText(GSText.STR_FOO, "foo"));
		text.SetParam(2, GSWindow.TC_RED);
		text.SetParam(3, 42);
		GSTown.SetText(0, text);

and

STR_TEST :{STRING} - {COLOUR}{NUM} is supposed to be 42
STR_FOO  :'{RAW_STRING}'

seems to show what I (and ScriptText validation) expect

encoded string is

~~Of course debug console is full of dbg: [misc] FormatString: Trying to read string parameter with wrong type~~
I disabled the debug line for game scripts.

PeterN · 2024-01-03T23:38:24Z

I've removed the second commit now because it was obviously wrong, and I think it can be raised in a separate PR.

rubidium42 · 2024-01-04T18:43:20Z

src/strings.cpp

-				/* raw_string can be(come) nullptr when the parameter is out of range and 0 is returned instead. */
-				if (raw_string == nullptr) {
-					builder += "(invalid RAW_STRING parameter)";
+				case SCC_RAW_STRING_POINTER: { // {RAW_STRING}
+					const char *raw_string = args.GetNextParameterString();
+					FormatString(builder, raw_string, args);
 					break;
 				}


It could also be nullptr when you, for whatever reason, pass nullptr to SetDParamStr. You can currently do that, and it will also trigger this code.
The question is whether we want to game to crash here, or should we maybe add an assert in SetDParamStr where applicable?

If it's still possible then I'll put it back to how it was I was misinformed :-)

Maybe replace the comment with /* raw_string can be nullptr. */, i.e. remove the bit about the parameter being out of range.

My bad, the comment was misleading :)

If not enough parameters are supplied for a string, then a value of 0 was used, which could result in incorrect information being displayed. Instead, throw an exception and include an error in the string.

github-advanced-security bot found potential problems Jan 3, 2024

View reviewed changes

LordAro reviewed Jan 3, 2024

View reviewed changes

src/strings.cpp Outdated Show resolved Hide resolved

LordAro reviewed Jan 3, 2024

View reviewed changes

src/strings.cpp Outdated Show resolved Hide resolved

PeterN force-pushed the invalid-parameters branch from 4fc2aaf to 438d233 Compare January 3, 2024 08:34

glx22 reviewed Jan 3, 2024

View reviewed changes

src/strings.cpp Outdated Show resolved Hide resolved

glx22 reviewed Jan 3, 2024

View reviewed changes

src/strings.cpp Outdated Show resolved Hide resolved

PeterN force-pushed the invalid-parameters branch from f9d844b to f5abdb5 Compare January 3, 2024 18:39

PeterN force-pushed the invalid-parameters branch from f5abdb5 to 10dfed1 Compare January 3, 2024 23:36

JGRennison mentioned this pull request Jan 4, 2024

Allow STRING/STRINGN parameter subspans to read all remaining available parameters in GS mode #11681

Merged

PeterN force-pushed the invalid-parameters branch from 10dfed1 to 2ce3c3c Compare January 4, 2024 18:09

rubidium42 reviewed Jan 4, 2024

View reviewed changes

PeterN force-pushed the invalid-parameters branch from 2ce3c3c to b2ba696 Compare January 4, 2024 19:07

Change: Don't handle 'missing' string parameters as 0.

def4fcd

If not enough parameters are supplied for a string, then a value of 0 was used, which could result in incorrect information being displayed. Instead, throw an exception and include an error in the string.

PeterN force-pushed the invalid-parameters branch from b2ba696 to def4fcd Compare January 4, 2024 19:17

rubidium42 approved these changes Jan 4, 2024

View reviewed changes

PeterN merged commit 7482f71 into OpenTTD:master Jan 4, 2024
20 checks passed

PeterN deleted the invalid-parameters branch January 4, 2024 20:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change: Don't handle 'missing' string parameters as 0. #11673

Change: Don't handle 'missing' string parameters as 0. #11673

PeterN commented Jan 3, 2024 •

edited

glx22 commented Jan 3, 2024 •

edited

PeterN commented Jan 3, 2024

rubidium42 Jan 4, 2024

PeterN Jan 4, 2024

rubidium42 Jan 4, 2024

glx22 Jan 4, 2024

Change: Don't handle 'missing' string parameters as 0. #11673

Change: Don't handle 'missing' string parameters as 0. #11673

Conversation

PeterN commented Jan 3, 2024 • edited

Motivation / Problem

Description

Limitations

Checklist for review

glx22 commented Jan 3, 2024 • edited

PeterN commented Jan 3, 2024

rubidium42 Jan 4, 2024

Choose a reason for hiding this comment

PeterN Jan 4, 2024

Choose a reason for hiding this comment

rubidium42 Jan 4, 2024

Choose a reason for hiding this comment

glx22 Jan 4, 2024

Choose a reason for hiding this comment

PeterN commented Jan 3, 2024 •

edited

glx22 commented Jan 3, 2024 •

edited