Another Engine Crasher...

[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.

[leileilol]

Thanks i'll keep that in mind!! Stability seems underrated in all these quake engines these days, so much volatile going on

[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.

[szo]

Bumping the array size is not a solution. Make COM_FileBase() buffer size safe. (see quakespasm svn r554, for e.g.)

[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;
    }
}


[szo]

like this ?
[snip]

I'd say just add an size_t outsize param to the function and then manipulate accordingly

[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.

[szo]

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]

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).

[revelator]

I kinda remember i used this version because the old one would bitch like hell with my pk3 code hmm.

[szo]

[...] 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]

aye that was it damn thing just started looping untill it crashed.
a safer version is definatly a must in those cases.