Fixed: backslash characters in savegames skips string chars

[mankrip]

ED_NewString had a bug in its newline parser, which always skipped the first character after a backslash even when it's not the 'n' char. This resulted in string corruption across subsequent saves & loads in the same gameplay session.

[sezero]

Patch is utterly broken: looks like copy+paste'd from an incompatible work.

[mankrip]

Patch is utterly broken: looks like copy+paste'd from an incompatible work.

Utterly broken how?

This function in QuakeSpasm is identical to its vanilla Quake code, and its only purpose is to convert escape sequenced newlines into newline characters. It doesn't try to convert any other escaped characters, in which case the characters should be parsed raw. This is all this patch does, it doesn't break anything.

[sezero]

Utterly broken how?

There is no local new variable here, the procedure had been changed long ago to call PR_AllocString and return the string num instead of the string itself and your patch reverts all of that and leaves num uninitialized.

This function in QuakeSpasm is identical to its vanilla Quake code,

No it is not: see above

and its only purpose is to convert escape sequenced newlines into newline characters. It doesn't try to convert any other escaped characters, in which case the characters should be parsed raw. This is all this patch does, it doesn't break anything.

Did you even attempt to compile?

In any case, the actual intended change should be something like the following I think:

diff --git a/Quake/pr_edict.c b/Quake/pr_edict.c
index cc12171..0aed418 100644
--- a/Quake/pr_edict.c
+++ b/Quake/pr_edict.c
@@ -769,13 +769,11 @@ static string_t ED_NewString (const char *string)
 
 	for (i = 0; i < l; i++)
 	{
-		if (string[i] == '\\' && i < l-1)
+		if (i < l-1 && string[i] == '\\' && string[i + 1] == 'n')
 		{
+			// convert both characters into a single newline character
 			i++;
-			if (string[i] == 'n')
-				*new_p++ = '\n';
-			else
-				*new_p++ = '\\';
+			*new_p++ = '\n';
 		}
 		else
 			*new_p++ = string[i];

[mankrip]

There is no local new variable here, the procedure had been changed long ago to call PR_AllocString and return the string num instead of the string itself and your patch reverts all of that and leaves num uninitialized.

I see now, you're right. I copied the entire function from my code without the variable declarations (which I didn't modify), when I should have copied just the for loop. Sorry for my mistake, and thanks for pointing it out. :)

[sezero]

OK.

In any case, @ericwa: Should we apply the patch (the corrected one I inlined above) to mainstream?

[ericwa]

From what I can see, the old code converts:

1. \n to linefeed
2. \\ to \
3. \ followed by any other char to \

Case 3 does seem sketchy, but the patch in https://github.com/ericwa/Quakespasm/pull/17#issuecomment-1956565862 drops cases 2. and 3. I'm not sure this is obviously the correct thing to do, why are we dropping case 2? Could there be side effects from dropping case 3?

I'm lacking the context for where ED_NewString is run in the save/load process at the moment.

[sezero]

@mankrip: I'm not applying the patch for now, per @ericwa's comments. If there is a reproducible case to demonstrate the issue, it'd be great.

[mankrip]

Start the game, then load a map using this command: map ..\..\mod_dir\maps\mapname

Create a savegame, then load it:

save path_test.sav
load path_test.sav

Now save again: save path_test_2.sav

And compare both saves in a text editor. The "mapname" global in the second save is corrupted: "mapname" "..\.\od_dir\aps\apname"

[sezero]

Ugghhhh -- is such a command ever allowed? And it looks exaggerated to me. @ericwa what do you think?

[mankrip]

That command does work, even in WinQuake.

This engine bug affects all mods that uses this hack from Preach, including the Copper mod: https://tomeofpreach.wordpress.com/2015/09/30/fixing-runes-and-restart-with-qc/

Starting Copper, entering a map with relative pathnames and "restarting" the map upon death after multiple saves & loads will crash the game to the console because the "mapname" global var is corrupted. I fixed this bug specifically to get such mods to stop crashing.

[mankrip]

By the way, the \n conversion alone will still make some relative pathnames crash the game. In my latest engine release I've also added some code to skip ED_NewString's conversion entirely on filename-related strings.