Buffer overflow in RE_Font_DrawString

[UncannyFish]

Line with overflow: https://github.com/mvdevs/jk2mv/blob/fc18d906b8684a2546e99bbced59806e8b62d262/src/renderer/tr_font.cpp#L880

Variable r is 1024 when server printing PLENTER message (maximum value for dropShadowText is 1023). Spotted by VS 2022 debugger.

[UncannyFish]

So I found at least 3 errors in this code: https://github.com/mvdevs/jk2mv/blob/fc18d906b8684a2546e99bbced59806e8b62d262/src/renderer/tr_font.cpp#L867-L880

1. while loop not checking for exceeding iCharLimit
2. i += 2 can jump over \0 in psText, so garbage copied to dropShadowText
3. r can be 1024 after while loop, this creates overflow in dropShadowText[r] = 0;

Probably error too, but I'm not sure: i is not checked for zero when accessing psText[i - 1]

I can't figure out the purpose of this code, but trying to fix it results in text shadows not working.

[Daggolin]

Thanks for the report and attempting to fix this.

The code was jk2 1.02's way of handling STYLE_DROPSHADOW output. I assume the idea was to remove all colorcodes by looping over the text, ignoring colorcodes and copying everything else into a separate buffer. Then the RE_Font_DrawString function calls itself recursively with a slight offset for the position and the "cleaned" string to create the dropshadow effect. The code doesn't properly handle special cases like two colorcodes right behind each other ("^5^5") , leading to colorcodes getting copied over into the dropshadow, creating the colored text shadows players consider a feature of 1.02, which seems to actually have been a bug. For 1.03+ the code was changed to set a global variable called gbInShadow and passing psText itself into the recursive call. With gbInShadow set the effects of colorcodes are not applied, thus leading to single colored dropshadows, as it was probably intended from the beginning.

When adding support for the colored dropshadows to JK2MV we weren't very familiar with the expected behavior and corner cases within the community, so we used the original 1.02 code. In retrospective I think it would've been better to approach this differently and I don't think there are any corner cases that would've behaved differently with a cleaner implementation.

Regarding the 3 errors:

1. I don't think this is an issue, because the iCharLimit is decremented and checked in the while loop further down the same function, so I don't think you need to check it when building the text for the drop shadows.

2. This is certainly an issue. I think I encountered it myself when experimenting with custom colorcodes. I do recall having a fix for it on a local branch for hex colors. Let me check for the code from an old test branch:

   //^blah stuff confuses shadows, so parse it out first
   while (psText[i] && r < 1024) {
   if (psText[i] == '^') {
       if ((i < 1 || psText[i - 1] != '^')) {
           // Don't jump over '\0'
           if ( !psText[i + 1] ) {
               break;
           }
           i += 2;
           // End if there is nothing left to copy afterwards
           if ( !psText[i] ) {
               break;
           }
       }
   }
   
   dropShadowText[r] = psText[i];
   r++;
   i++;
   }
   dropShadowText[r] = 0;

3. That looks like an issue, too. Honestly, I think the dropShadowText buffer shouldn't be using a fixed size, but rather allocate the memory dynamically, considering psText is a pointer from another function without a maximum length.
4. i is checked for i < 1 before accessing psText[i-1], so this shouldn't be an issue either as i should be at least 1 when trying to access previous characters.

I don't think it's a good idea to try and fix this broken code. I think it would be a lot better to use the 1.03+ code for the recursive call, but alter the bgInShadow behavior to only apply the effect when encountering a colorcode right after a colorcode when 1.02 dropshadows are desired. That should recreate the same effect, but without any fixed size or dynamically allocated buffers.

[UncannyFish]

Thank you for such an in-depth response. For now I just disabled mv_coloredTextShadows in my local build, so VS 2022 can stop throwing exceptions at me.