Another Engine Crasher...

Post tutorials on how to do certain tasks within game or engine code here.
Post Reply
mh
Posts: 2292
Joined: Sat Jan 12, 2008 1:38 am

Another Engine Crasher...

Post by mh »

In COM_LoadFile:

Code: Select all

char base[32];
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.
We had the power, we had the space, we had a sense of time and place
We knew the words, we knew the score, we knew what we were fighting for
leileilol
Posts: 2783
Joined: Fri Oct 15, 2004 3:23 am

Re: Another Engine Crasher...

Post by leileilol »

Thanks i'll keep that in mind!! Stability seems underrated in all these quake engines these days, so much volatile going on
i should not be here
Baker
Posts: 3666
Joined: Tue Mar 14, 2006 5:15 am

Re: Another Engine Crasher...

Post 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.
The night is young. How else can I annoy the world before sunsrise? 8) Inquisitive minds want to know ! And if they don't -- well like that ever has stopped me before ..
szo
Posts: 132
Joined: Mon Dec 06, 2010 4:42 pm

Re: Another Engine Crasher...

Post by szo »

Bumping the array size is not a solution. Make COM_FileBase() buffer size safe. (see quakespasm svn r554, for e.g.)
revelator
Posts: 2621
Joined: Thu Jan 24, 2008 12:04 pm
Location: inside tha debugger

Re: Another Engine Crasher...

Post 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;
    }
}
Productivity is a state of mind.
szo
Posts: 132
Joined: Mon Dec 06, 2010 4:42 pm

Re: Another Engine Crasher...

Post by szo »

reckless wrote:like this ?
[snip]
I'd say just add an size_t outsize param to the function and then manipulate accordingly
revelator
Posts: 2621
Joined: Thu Jan 24, 2008 12:04 pm
Location: inside tha debugger

Re: Another Engine Crasher...

Post 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.
Productivity is a state of mind.
szo
Posts: 132
Joined: Mon Dec 06, 2010 4:42 pm

Re: Another Engine Crasher...

Post 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';
	}
}
mh
Posts: 2292
Joined: Sat Jan 12, 2008 1:38 am

Re: Another Engine Crasher...

Post 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).
We had the power, we had the space, we had a sense of time and place
We knew the words, we knew the score, we knew what we were fighting for
revelator
Posts: 2621
Joined: Thu Jan 24, 2008 12:04 pm
Location: inside tha debugger

Re: Another Engine Crasher...

Post by revelator »

I kinda remember i used this version because the old one would bitch like hell with my pk3 code hmm.
Productivity is a state of mind.
szo
Posts: 132
Joined: Mon Dec 06, 2010 4:42 pm

Re: Another Engine Crasher...

Post 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 )
revelator
Posts: 2621
Joined: Thu Jan 24, 2008 12:04 pm
Location: inside tha debugger

Re: Another Engine Crasher...

Post by revelator »

aye that was it :) damn thing just started looping untill it crashed.
a safer version is definatly a must in those cases.
Productivity is a state of mind.
Post Reply