|
Notes |
|
|
(0002977)
|
|
TIHan
|
|
2012-03-29 01:45
|
|
The issue is this:
sv_main.cpp
// This is the music the loaded map is currently using.
static char g_szMapMusic[16];
g_szMapMusic cannot hold more than necessary.
What is the best practice to handle this? |
|
|
|
(0002980)
|
|
Torr Samaho
|
2012-03-29 01:58
(edited on: 2012-03-29 01:59) |
|
Use memcpy and terminate manually instead of sprintf. Alternative: Use std::string instead of a fixed length char array.
|
|
|
|
|
|
is it bad using snprintf with second argument 16? |
|
|
|
|
|
For security reasons one should only use snprintf if format stuff like %d is supposed to have an effect. |
|
|
|
(0002999)
|
|
Edward-san
|
2012-03-29 13:07
(edited on: 2012-03-29 13:45) |
|
Like this:
diff -r 7efcdc068118 src/sv_main.cpp
--- a/src/sv_main.cpp Sun Mar 25 18:09:08 2012 -0400
+++ b/src/sv_main.cpp Thu Mar 29 15:06:56 2012 +0200
@@ -3514,7 +3514,10 @@
void SERVER_SetMapMusic( const char *pszMusic )
{
if ( pszMusic )
- sprintf( g_szMapMusic, "%s", pszMusic );
+ *((char *) mempcpy (g_szMapMusic, pszMusic, 15)) = '\0';
else
g_szMapMusic[0] = 0;
}
?
[edit]thanks to AlexMax I found a hacky code which needs one line change! ;D
btw: tabs are not rendered correctly in the code snippet, weird.
Tested, it works fine on me. I connected a client and changemus changes the client music correctly.
Also, I was thinking about changing "g_szMapMusic[0] = 0;" to "g_szMapMusic[0] = '\0';", nothing serious.
|
|
|
|
|
|
|
|
|
A mempcpy call like this will possibly read beyond the bounds of pszMusic (nobody guarantees that pszMusic has a length of at least 15), so one would need to replace 15 by min (strlen(pszMusic),15) and adjust the termination accordingly. This way the stuff get's pretty complicated though. Thinking about this I'd say it's better to use strncpy (sorry for not mentioning this earlier):
strncpy (g_szMapMusic,pszMusic,15);
g_szMapMusic[15]='\0';
should do it. The advantage of strncpy is that it will not read beyond strlen(pszMusic). |
|
|
|
(0003027)
|
|
TIHan
|
|
2012-03-30 00:50
|
|
Just curious, is it wise to use sizeof here to determine the length?:
strncpy (g_szMapMusic,pszMusic,sizeof(g_szMapMusic)-1);
g_szMapMusic[sizeof(g_szMapMusic)-1]='\0';
I ask because someone could come in and change the size of g_szMapMusic[16] to something like g_szMapMusic[8] for whatever reason. |
|
|
|
|
|
Yes, that would make the code more robust. |
|
|
|
|
|
|
|
|
|
|
|
|
|
Yes, it seems that you fixed the issue. Great job. |
|
screenshot.png (18,573) 2012-03-28 18:59
737.diff (373) 2012-03-29 21:41