Problems with XWE TEXTURE1 lump [title changed]

While trying to repackage some old, notoriously problematic megawads/TCs (Quest for the Necronomicon, Obituary), a common problem I stumbled upon was that both had malformed texture lumps that contained characters beyond the null terminator mark, see example here:

http://www.doomworld.com/vb/showthread.php?s=&postid=1001630#post1001630

In that case, a certain texture "YELMIX" was actually stored in the TEXTURE1 lump as "YELMIX 2", where the "space" was actually the null terminator. This problem was present on several textures, and in the case of Quest for the Necronomicon, also in many patch names referred by textures.

When the "YELMIX" texture is recalled from a linedef, most modern source ports (prBoom+ and ZDoom did this) seem to bomb on the grounds that they can't match "YELMIX" with "YELMIX[null]2" with a string comparison (and similarly for other textures). The problem can extend to patches and floors in a similar way.

Yet, vanilla Doom obviously had no problem using the malformed entries with either TC.

My theory is that this is due to how string comparisons are performed in the DOS .exe (fully left-to-right scans until the first null terminator, so the bogus/trailing text is ignored), while modern source ports use modified/optimized string comparisons which however are fooled into believing that strings mismatch (due to encountering valid characters beyond the real, left-most null terminator).

Now, Chocolate Doom in particular, since it's aiming at reproducing the DOS executable's behavior as closely as possible, should probably implement a string comparison function that is guaranteed to behave like the DOS one (e.g. perform no optimizations like starting from the end of a length-limited comparison).

Since texture name matching is done in R_CheckTextureNumForName (char *name) the "offending" library function should be strncasecmp, used as follows:

if (!strncasecmp (textures[i]->name, name, 8) )

Now, in some optimized implementations this might return a false negative on two strings "YELMIX[null]2" and "YELMIX", since it will attempt starting from the 8th character, while unoptimized ones will correctly give a positive. At least that's what I'm imagining.

What's more disturbing is the knowledge that there probably are more such modifications out there, plagued by the same problem which went under the radar for years, which can only be fixed by cleaning up the TEXTURE1 lumps, renaming patch and floor names manually or by making source ports tolerant/modifying their string comparison functions.

I was not able to reproduce your results in prboom-plus. It uses strncasecmp(foo, bar, 8), and I made a test wad with specifically malformed sidedef texture names but it read them properly as I would expect it to if strncasecmp worked right.

Doing specific tests witn MinGW also seem to work OK for me, yet in practice me and other people who tested my Obituary repackaging had problems with malformed TEXTURE1 lumps (the malformed text was in TEXTURE1 only, not in the sidedefs themselves), which went away as soon as I corrected the texture name definitions. Maybe it has to do with how texture names are read from the lump during texture initialization.

Compare the behavior of Chocolate Doom and prBoom+ on my current (fixed names) and my initial repackaging of Obituary.

In particular, go to MAP11 and look for the yellow key door. With the unfixed version, you will get a HOM in prBoom+ (where the yellow skull textures are supposed to be) and a crash with error from chocolate doom, complaining it cannot find YELMIX.

These problems were even worse in Quest for the Necronomicon, which also had malformed patch names inside the TEXTURE1 lump.

First test only used malformed texture names in SIDEDEFS. Created another test wad including TEXTUREx resource with malformed texture names in there. Still can't reproduce any problem at all.

This means that you DON'T stumble upon a HOM in the situation described above? (using the obticfixold.zip download)

The offending door is on MAP11, X: 296, Y: -1496

I stumbled upon it with prBoom+ 2.5.1.1, from the binary installer. BTW, even DoomBuilder has trouble displaying the YELMIX texture if using the unfixed version (displays "unknown texture" if I leave it as is).

Maes said:

This means that you DON'T stumble upon a HOM in the situation described above? (using the obticfixold.zip download)

No, I do get that error. But I'm unable to reconcile it with your explanation of the problem, as testing strncasecmp() by itself it seems to work properly, and my attempts to create a minimal test case wad failed.

Well, I pretty much gave up on that explanation after checking it out myself with MinGW. However the problem remains, as does the fact that vanilla Doom apparently has had no problem with it all these years.

Perhaps the discrepancy isn't created at the point that I indicated?

The only certain aspect is that when reading the texture lumps, C/C++ ports just do a raw struct copy without sanitizing the strings, so the malformed strings surely do get in memory intact, with bad terminators and all. But this should not be enough to foil strncasecmp...in theory. At least it doesn't affect the MinGW version....

I examined obticfix.wad (HOM version)with a hex editor, and in that wad, YELMIX is actually "YELMIX 2", with the space being a space, not a null terminator. I also examined obtic1.wad (the real original) in a hex editor, and in that version YELMIX is actually "YELMIX\x002".

You must have broken the texture lump by editing it in XWE or a similarly braindead editor.

Heh then it's a XWE problem after all. I will to see if copying the original lump intact (hopefully without it being silently auto-edited) will fix this. Still....I think it would be better if the lump contained no such bogosity at all.

Edit: yup, copying the lump directly by avoiding any triggering whatsoever of XWE's texture editor seemed to fix it. The bad thing is that it affects ALL strings at the same time, even those that went totally untouched, so once you trigger this, you must either fix everything or use a raw copy. Either way, it's still a bad practice to leave uncleaned strings in there.

Maes said:

Heh then it's a XWE problem after all.

There was a post not too long ago that explained it, but I'm too lazy to search, so I'll just describe it here:

On loading the texture lump into memory, XWE takes any NUL characters in any texture name and replaces them with spaces.
On saving the texture lump, XWE takes any trailing spaces and replaces them with NUL.

Here is Gez's explanation

Maes said:

Heh then it's a XWE problem after all. I will to see if copying the original lump intact (hopefully without it being silently auto-edited) will fix this. Still....I think it would be better if the lump contained no such bogosity at all.

I've found DeuTex to be a handy tool for sanitizing TEXTURE* and PNAMES lumps, just hope that problem doesn't extend to the SIDEDEFS lumps.

XWE is also known to replace perfectly valid PWADs with statistically random garbage, amongst other things, so this really comes as no shock to me whatsoever.

People should really stop using XWE...

Quasar said:

statistically random garbage

Perhaps the XWE codebase could be re-purposed as a cryptographic random number generator?

Well, if the problem is really in THAT ONE SPOT only, I could give my 1337 (albeit rusty) Delphi skillz a polish and fix it to do proper C-like (aka null-terminated, left-to-right scanned) string parsing and solve everybody some grief.

So I've read through the entire thread, and I'm not sure what (if anything) I need to fix...

Is there a specific identifiable difference between Vanilla and Chocolate or not? Does it happen on all platforms, or only some?

No no, it's all OK. Turned out that if anything needs fixing, it's XWE's (broken) parsing of strings in the TEXTURE1 lump (pulls out dusty Delphi 7 IDE from uni).

If you could make it so that XWE does not constantly rewrite the opened wad, completely fucking it up in the process if it's opened in another application (including another instance of XWE), that would be just gravy.

Heh I just re-opened the XWE project, and remembered why I had just kind of gave up on continuing/improving it: it's missing some dependencies (including what appears to be a custom-written string manipulation unit) and an external ZipMaster one, other than being designed for Delphi 5 and not 7. It's a chore just getting it to compile as-is.

Edit: the source Csabo released is not complete: it's missing A LOT of external dependencies, some of which are quite hard to find (e.g. the CodeMax highlighting editor's Delphi CodeMax_TLB headers) or were probably custom-written by Csabo himself ('Stringz' string utilities, 'Evaluation' numerical evaluation routines etc.) and not included in the released source code.

I can attempt to recreate some of the missing routines or replace the ZipMaster with a more recent versions, but even then it's missing just too much stuff to even get to compile.

Maes said:

I can attempt to recreate some of the missing routines or replace the ZipMaster with a more recent versions, but even then it's missing just too much stuff to even get to compile.

Why not contribute to SLADE 3 instead?

Maes said:

Heh I just re-opened the XWE project, and remembered why I had just kind of gave up on continuing/improving it: it's missing some dependencies (including what appears to be a custom-written string manipulation unit) and an external ZipMaster one, other than being designed for Delphi 5 and not 7. It's a chore just getting it to compile as-is.

Csabo said:

It's written in Delphi 5 (I know, ancient). One quick note: if PNGImage/CodeMax/Zip handlers give you trouble, you're probably better off commenting out anything that deals with those parts. I don't think those are essential to WAD editing, and could be replaced with other libraries.

So yeah.

AlexMax said:

Why not contribute to SLADE 3 instead?

The real problem is to get people to stop recommending XWE. Because as long as it's promoted despite having been left unmaintained for five years now (last release was in 2006), people will keep using it, and stumble unto its bugs, that won't ever be patched.

Gez said:

Stuff Csabo said about commenting out "troublesome" parts of the code

Easier said than done. Most of the CodeMax parts may be indeed commentable out/$IFDEF-ed with the preprocessor, and the Zip/Gif/Png parts are easy enough to recover online and include in the project. But some parts are so ingrained in the code that you can't simply "comment them out" without a major rewrite, and unless you use the particular versions used by Csabo, you will need to rewrite some parts anyway.

The trouble is that there are some string manipulation and number evaluation stuff that are NOT included in the source and from the looks of it are custom-written libraries, and are used all over the code. These need recreation based on their names and guesstimating their possible functionality (e.g. KeepFromLeft, PosR, Zero, Comma,IsNumber, SafeVal and others), as there's no documentation about them, nor any mention that they were part of a known library.

The fact that it's for Delphi 5 is not such aa proplem per-se (Delphi 7 handles that fine), as much as all that missing stuff.

Gez said:

The real problem is to get people to stop recommending XWE. Because as long as it's promoted despite having been left unmaintained for five years now (last release was in 2006), people will keep using it, and stumble unto its bugs, that won't ever be patched.

Well, WinTex lived on for much longer ;-)