Unsequenced modification and access to 'n'

[QwazyWabbit]

While hacking TMG mod I found this little gem:

Code: [Select]

// From QDevels, posted by outlaw
void convert_string(char *src, char start, char end, char add, char *dest)
{
    int n = -1;
    while ((dest[++n] = src[n]))
        if ((dest[n] >= start) && (dest[n] <= end) && (dest[n] != '\n'))
            dest[n] += add;
}

/* Examples of convert_string usage
 {
 char  text[] = "abcdefgABCDEFG1234567\n\0";
 convert_string(text, 'a', 'z', ('A'-'a'), text); // a -> A
my_bprintf (PRINT_CHAT, "text = %s\n", text);
 convert_string(text, 'A', 'Z', ('a'-'A'), text); // A -> a
my_bprintf (PRINT_CHAT, "text = %s\n", text);
 convert_string(text, 0, 127, 128, text); // white -> green
my_bprintf (PRINT_CHAT, "text = %s\n", text);
 convert_string(text, 128, 255, -128, text); // green -> white
my_bprintf (PRINT_CHAT, "text = %s\n", text);
 }
 */

What first caught my eye was the warning from the compiler per the subject line but second was the fun and exciting int n = -1; initialization. OK, so we're being really 1337 coder and were starting the index off below the array bounds of dest and the ++n is going to make it 0 before it puts the value of src[n] into the destination.... or does it?

The warning is about that dest[++n] thing. This is a reliance on undefined behavior in C and it can be a nasty bug. It will work fine in one platform and fail in another and it can take some time to fix if it's buried deep enough. As it was, Microsoft Visual Studio 2010 likes it and it works. It also works on Ubuntu and GCC 4.8.4 but it doesn't work on OS X, using clang/LLVM. Since Microsoft is migrating to using clang on their compiler suite you can expect this function will break when compiled on the new tools or in their Azure cloud. It may also fail on newer versions of GCC on other Linux distributions.

OK, so how to fix it? Answer: pointers instead of array indexes.

Code: [Select]

/**
 Replace characters in destination string.
 Parameter 'add' is added to each character found in source and result is placed in dest.
 Parameters 'start' and 'end' specify character range to replace.
 Source text must be a valid C string.
*/
//QwazyWabbit// A pointer version to eliminate undefined behavior.
void convert_string(char *src, char start, char end, char add, char *dest)
{
while ((*dest = *src)) {
if ((*dest >= start) && (*dest <= end) && (*dest != '\n'))
*dest += add;
src++, dest++;
}
}

In keeping with the existing 1337ness of the code, I made good use of the comma operator.

If your projects contain this original Qdevels/outlaw function I urge you replace it ASAP. You'll know the old one is broken when your Q2 strings and chats suddenly become blank when you port your mod instead of green or white.

In addition, I offer the following wrapper functions to use in place of the rather complicated direct uses of the 'convert_string' function:

Code: [Select]

/**
 Set msb in specified string characters, copying them to destination.
 Text must be a valid C string.
 Source and destination can be the same.
 If dest == NULL the action occurs in-place.
 */
void highlight_text (char *src, char *dest)
{
if (dest == NULL)
dest = src;
convert_string(src, 0, 127, 128, dest); // white -> green
}

/**
 Clear msb in specified string characters, copying them to destination.
 Text must be a valid C string.
 Source and destination can be the same.
 If dest == NULL the action occurs in-place.
 */
void white_text (char *src, char *dest)
{
if (dest == NULL)
dest = src;
convert_string(src, 128, 255, -128, dest); // green -> white
}

/**
 Make text uppercase.
 Text must be a valid C string.
 Source and destination can be the same.
 If dest == NULL the action occurs in-place.
 */
void toupper_text(char *src, char *dest)
{
if (dest == NULL)
dest = src;
convert_string(src, 'a', 'z', ('A'-'a'), dest); // a -> A
}

/**
 Make text lowercase.
 Text must be a valid C string.
 Source and destination can be the same string.
 If dest == NULL the action occurs in-place.
 */
void tolower_text(char *src, char *dest)
{
if (dest == NULL)
dest = src;
convert_string(src, 'A', 'Z', ('a'-'A'), dest); // A -> a
}


Testbed code:

Code: [Select]

#include <stdio.h>

int main(int argc, const char * argv[])
{
char otext[] = "abcdefgABCDEFG1234567";
char text[]  = "abcdefgABCDEFG1234567\n";
char target[sizeof text];

toupper_text(otext, target); // a -> A
printf ("otext   = %s\n", otext);
printf ("target  = %s\n", target);
toupper_text(text, NULL); // a -> A
printf ("text    = %s\n", text);

tolower_text(otext, target); // A -> a
printf ("otext   = %s\n", otext);
printf ("target  = %s\n", target);
tolower_text(text, NULL); // A -> a
printf ("text    = %s\n", text);

highlight_text(otext, target); // white -> green
printf ("otext   = %s\n", otext);
printf ("target  = %s\n", target);
highlight_text(text, NULL); // white -> green
printf ("text    = %s\n", text);

white_text(target, otext); // green -> white
printf ("target  = %s\n", target);
printf ("otext   = %s\n", otext);
white_text(text, NULL); // green -> white
printf ("text    = %s\n", text);

return 0;
}


[bvughtoonice]

i had a friend try setting up tmg on frebsd and he had that issue with the text. no players names etc.

[QwazyWabbit]

That's very interesting, OS X is based on FreeBSD. The compiler version will have the most influence of course. What version compiler does he have and what version OS?

The convert_string function is in g_utils.c in the TMG mod. You can copy-paste the code from here into your copy. The archive I posted May 7 has the bug, the one I posted May 8 has a version of my replacement and should work correctly.