Page 1 of 1
Another Engine Crasher...
Posted: Tue Dec 27, 2011 12:56 am
by mh
In COM_LoadFile:
That just won't do at all. Give COM_LoadFile a filename longer than 32 chars and you go down in flames. Change it from 32 to MAX_OSPATH and we're living in happier days.
Re: Another Engine Crasher...
Posted: Tue Dec 27, 2011 1:51 am
by leileilol
Thanks i'll keep that in mind!! Stability seems underrated in all these quake engines these days, so much volatile going on
Re: Another Engine Crasher...
Posted: Tue Dec 27, 2011 3:20 am
by Baker
Thanks for the info. I know I do not like the vague "this buffer size is X bytes" hardcoding stuff and have sought to try to reduce its usage.
Re: Another Engine Crasher...
Posted: Tue Dec 27, 2011 9:11 am
by szo
Bumping the array size is not a solution. Make COM_FileBase() buffer size safe. (see quakespasm svn r554, for e.g.)
Re: Another Engine Crasher...
Posted: Tue Dec 27, 2011 1:03 pm
by revelator
like this ?
Code: Select all
void COM_FileBase (const char *in, char *out)
{
const char *start, *end;
int length;
if (!(end = strrchr(in, '.')))
{
end = in + strlen(in);
}
if (!(start = strrchr(in, '/')))
{
start = in;
}
else
{
start += 1;
}
length = end - start;
if (length < 0)
{
strlcpy (out, "?empty filename?", 0);
}
else if (length == 0)
{
out[0] = 0;
}
else
{
if (length > 31)
{
length = 31;
}
memcpy (out, start, length);
out[length] = 0;
}
}
Re: Another Engine Crasher...
Posted: Tue Dec 27, 2011 1:11 pm
by szo
reckless wrote:like this ?
[snip]
I'd say just add an size_t outsize param to the function and then manipulate accordingly
Re: Another Engine Crasher...
Posted: Tue Dec 27, 2011 4:33 pm
by revelator
dont remember where i got the above function but its been in my realm engine for years now.
the size_t idea might come in handy in my own engine since this ones a bit bloated (works though)
but it needs a strlcpy implementation which does not exist normally on windows, so i used the one from darkplaces.
Re: Another Engine Crasher...
Posted: Tue Dec 27, 2011 5:21 pm
by szo
reckless wrote:dont remember where i got the above function but its been in my realm engine for years now.
the size_t idea might come in handy in my own engine since this ones a bit bloated (works though)
but it needs a strlcpy implementation which does not exist normally on windows, so i used the one from darkplaces.
I use strlcpy, too, from OpenBSD. Here's what I'm using, which was adapted from quakeforge:
Code: Select all
/*
============
COM_FileBase
take 'somedir/otherdir/filename.ext',
write only 'filename' to the output
============
*/
void COM_FileBase (const char *in, char *out, size_t outsize)
{
const char *dot, *slash, *s;
s = in;
slash = in;
dot = NULL;
while (*s)
{
if (*s == '/')
slash = s + 1;
if (*s == '.')
dot = s;
s++;
}
if (dot == NULL)
dot = s;
if (dot - slash < 2)
q_strlcpy (out, "?model?", outsize);
else
{
size_t len = dot - slash;
if (len >= outsize)
len = outsize - 1;
memcpy (out, slash, len);
out[len] = '\0';
}
}
Re: Another Engine Crasher...
Posted: Tue Dec 27, 2011 7:32 pm
by mh
szo is correct here, but with the observation that in this particular case you're not going to be exceeding the limits of MAX_OSPATH to begin with (and within a pak file you'll never exceed 56 chars - including the NULL terminator - as the format won't allow you to). Bumping the array size is good enough for a quick and dirty fix (but in both cases, and as COM_FileBase is used to generate a hunk tag, you need to ensure that copying off the hunk tag to your hunk block is also robustly handled).
When it comes to stripping extensions/etc I personally prefer to work backwards through the string, as it more robustly catches the case where a directory name may have a '.' in it (and it's easier to catch the relevant path delimiter this way too).
Re: Another Engine Crasher...
Posted: Tue Dec 27, 2011 11:06 pm
by revelator
I kinda remember i used this version because the old one would bitch like hell with my pk3 code hmm.
Re: Another Engine Crasher...
Posted: Wed Dec 28, 2011 7:47 am
by szo
reckless wrote:[...] the old one would bitch like hell with my pk3 code hmm.
That's possibly because the unmodified quake version of the function has a nasty oversight.
(See this old commit:
http://quakespasm.svn.sourceforge.net/v ... revision=3 )
Re: Another Engine Crasher...
Posted: Wed Dec 28, 2011 11:12 am
by revelator
aye that was it
damn thing just started looping untill it crashed.
a safer version is definatly a must in those cases.