grfcodec crashes with filenames longer than 24 characters

[PhilSophus]

I just had an issue with grfcodec crashing on a trivial NFO file (with the message "*** stack smashing detected ***"), which passed renum without problems. After some investigation it turned out that the length of the filename was the problem:

This one crashed:

Code: Select all

grfcodec -e philsophus_building_base_costs.grf .

This one didn't (same file just copied):

Code: Select all

grfcodec -e ps_bbc.grf .

Further investigation showed that the magic length where it starts crashing is 25 characters including extension. It already seems to happen before the NFO file is actually opened, as it doesn't complain about a missing one.

OS: Kubuntu 8.04.1
GRFCodec version 0.9.10 r2055, freshly checked out and compiled from SVN

gdb stack trace:

Code: Select all

#0  0xb7eed410 in __kernel_vsyscall ()
#1  0xb7c81085 in raise () from /lib/tls/i686/cmov/libc.so.6
#2  0xb7c82a01 in abort () from /lib/tls/i686/cmov/libc.so.6
#3  0xb7cb9b7c in ?? () from /lib/tls/i686/cmov/libc.so.6
#4  0xb7d43138 in __fortify_fail () from /lib/tls/i686/cmov/libc.so.6
#5  0xb7d430f0 in __stack_chk_fail () from /lib/tls/i686/cmov/libc.so.6
#6  0x0804d1e9 in getspritefilename (filename=0x80677c0 "philsophus_building_base_costs.new",
    basefilename=0xbfec39c6 "philsophus_building_base_costs.grf", subdirectory=0xbfec3354 "", ext=0x805ef47 ".new", spriteno=-1)
    at grfcomm.c:118
#7  0x08007374 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)

[FooBar]

The precompiled Windows version shows similar behaviour, although it doesn't crash; it just doesn't do anything useful. The same length limit applies to file names in real sprites within nfo code.

[michael blunck]

Well. Anyhow, what´s wrong with "ps_bbc.grf"? I´d rather like to type that file name than "philsophus_building_base_costs.grf". The days of COBOL should have passed and Unix once started with short (but memorable) names ...

regards
Michael

[PhilSophus]

Well. Anyhow, what´s wrong with "ps_bbc.grf"?

Not seeing at all, what it does? This is hardly a self-descriptive name.

The days of COBOL should have passed [...]

So, have the days of DOS with 8.3 filenames

I´d rather like to type that file name than "philsophus_building_base_costs.grf".

Normally, I need to type it only once as auto-complete works quite well on my system even with non-unique prefixes. I prefer to read names like the latter, though I admit that was a bit extreme in length.


Back to topic, I assume the problem is caused by a buffer overrun. As a program should accept filenames of any length valid for the underlying OS, something like PATH_MAX (which to my knowledge is part of POSIX and should hopefully exist for every supported platform) should be used for filename buffer sizes.

I hope, this is the appropriate place for reporting grfcodec bugs.

[DaleStan]

So, have the days of DOS with 8.3 filenames

Except that they haven't. DOS TTDPatch cannot read GRF files that longer than 8+3. Adding "_win" to the DOS version gives 12+3 -- 16 characters.

If you submit a patch, I'll commit it, but it's not worth my time to fix grfcodec. I like having all my GRF files match at least one of the (DOS wildcard) patterns "????????w.grf" and "????????.grf".

You might convince me to make GRFCodec refuse to touch over-length filenames, though.

[FooBar]

IMO, adding the max filename length to the documentation would suffice. A warning within the program itself when people still try to use longer filenames would be better, but if people just RTFM...

[DJ Nekkid]

but you know FooBar, spending 4+ hrs of testing can save 10 minutes of reading the manual

[PhilSophus]

So, have the days of DOS with 8.3 filenames

Except that they haven't. DOS TTDPatch cannot read GRF files that longer than 8+3. Adding "_win" to the DOS version gives 12+3 -- 16 characters.

Okay, I forgot about the original game being (also) DOS.

So, do I get you right that it's just "luck" that it doesn't crash between 16 and 24 characters, because it just overwrites "unimportant" stuff?

If you submit a patch, I'll commit it, but it's not worth my time to fix grfcodec. I like having all my GRF files match at least one of the (DOS wildcard) patterns "????????w.grf" and "????????.grf".

I might do so at some time, but it's quite low on a long (Open)TTD(Patch) todo list (until this item it was an OpenTTD todo list ).

You might convince me to make GRFCodec refuse to touch over-length filenames, though.

IMHO, it would indeed be a good quick fix if it just refused filenames above the limit with an error message. It took me quite some time to find out what the problem was as I, as an NFO novice, assumed there was something wrong with my NFO (despite renum accepting it). I already had a post with my NFO code ready, when pasting the command line and looking at the gdb stack trace again I got the impression that the problem occured before actually reading the file.

As FooBar said even mentioning it as "known issue" in the documentation would help a lot (In that case may I also suggest to rename grfcodec.txt to README or README.txt or putting the txt files into a doc subdirectory? I only found it by accident after having posted), though I think checking the length of a parameter before copying around should not be that much work, either. A good compromise would be to mention it in the built-in help. For most other options it mentions limits and a relatively short filename limit is not exactly what you would expect.

[DaleStan]

So, do I get you right that it's just "luck" that it doesn't crash between 16 and 24 characters, because it just overwrites "unimportant" stuff?

The array is 20 characters long for the part of the name before the extension. There is another 6-character array for the extension. http://svn.ttdpatch.net/trac/browser/mi ... comm.c#L50
I don't think, therefore, that luck has much to do with it, unless it's luck that it doesn't detect a stack smash when just the trailing null overflows the buffer.