Zandronum Chat on our Discord Server Get the latest version: 3.2
Source Code
My View | View Issues | Change Log | Roadmap | Zandronum Issue Support Ranking | Rules | My Account
  

View Issue Details [ Jump to Notes ] [ Issue History ] [ Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0000737Zandronum[All Projects] Bugpublic2012-03-28 18:592018-09-30 19:52
Reporterunknownna 
Assigned ToEdward-san 
PrioritynormalSeveritycrashReproducibilityalways
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version98d 
Target VersionFixed in Version1.0 
Summary0000737: Server "changemus" crash
DescriptionYou can crash servers with the changemus command.
Steps To Reproduce1. Start a server with "skulltag.exe -host".
2. "changemus 111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111" in the console.
3. "a" in the console.
Additional InformationIt looks like sv_colorstripmethod automatically changes to 2 after entering the changemus command into the server console.
Attached Filespng file icon screenshot.png [^] (18,573 bytes) 2012-03-28 18:59


diff file icon 737.diff [^] (373 bytes) 2012-03-29 21:41 [Show Content]
diff file icon 737-2.diff [^] (427 bytes) 2012-03-30 11:13 [Show Content]

- Relationships
related to 0000954closed Changing music via path crashes servers 

-  Notes
User avatar (0002977)
TIHan (reporter)
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?
User avatar (0002980)
Torr Samaho (administrator)
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.
User avatar (0002989)
Edward-san (developer)
2012-03-29 09:33

 is it bad using snprintf with second argument 16?
User avatar (0002995)
Torr Samaho (administrator)
2012-03-29 12:00

 For security reasons one should only use snprintf if format stuff like %d is supposed to have an effect.
User avatar (0002999)
Edward-san (developer)
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.
User avatar (0003020)
Edward-san (developer)
2012-03-29 21:42

 Added the patch file.
User avatar (0003026)
Torr Samaho (administrator)
2012-03-30 00:35

 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).
User avatar (0003027)
TIHan (reporter)
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.
User avatar (0003028)
Torr Samaho (administrator)
2012-03-30 01:07

 Yes, that would make the code more robust.
User avatar (0003044)
Edward-san (developer)
2012-03-30 11:13

 New patch attached.
User avatar (0003062)
Torr Samaho (administrator)
2012-03-31 16:36

 Patch added.
User avatar (0003086)
unknownna (updater)
2012-04-01 19:21

 Yes, it seems that you fixed the issue. Great job.

Issue Community Support
This issue is already marked as resolved.
If you feel that is not the case, please reopen it and explain why.
Supporters: No one explicitly supports this issue yet.
Opponents: No one explicitly opposes this issue yet.

- Issue History
Date Modified Username Field Change
2012-03-28 18:59 unknownna New Issue
2012-03-28 18:59 unknownna File Added: screenshot.png
2012-03-28 18:59 unknownna Status new => confirmed
2012-03-28 23:28 Dusk Assigned To => Dusk
2012-03-28 23:28 Dusk Status confirmed => assigned
2012-03-29 01:45 TIHan Note Added: 0002977
2012-03-29 01:45 TIHan Status assigned => feedback
2012-03-29 01:58 Torr Samaho Note Added: 0002980
2012-03-29 01:59 Torr Samaho Note Edited: 0002980 View Revisions
2012-03-29 09:33 Edward-san Note Added: 0002989
2012-03-29 12:00 Torr Samaho Note Added: 0002995
2012-03-29 13:07 Edward-san Note Added: 0002999
2012-03-29 13:25 Edward-san Note Edited: 0002999 View Revisions
2012-03-29 13:40 Edward-san Note Edited: 0002999 View Revisions
2012-03-29 13:41 Edward-san Note Edited: 0002999 View Revisions
2012-03-29 13:45 Edward-san Note Edited: 0002999 View Revisions
2012-03-29 19:08 Dusk Assigned To Dusk =>
2012-03-29 21:38 Edward-san Assigned To => Edward-san
2012-03-29 21:38 Edward-san Status feedback => assigned
2012-03-29 21:41 Edward-san File Added: 737.diff
2012-03-29 21:42 Edward-san Note Added: 0003020
2012-03-30 00:35 Torr Samaho Note Added: 0003026
2012-03-30 00:50 TIHan Note Added: 0003027
2012-03-30 01:07 Torr Samaho Note Added: 0003028
2012-03-30 11:13 Edward-san Note Added: 0003044
2012-03-30 11:13 Edward-san File Added: 737-2.diff
2012-03-31 16:36 Torr Samaho Note Added: 0003062
2012-04-01 19:21 unknownna Note Added: 0003086
2012-04-01 19:24 Torr Samaho Status assigned => resolved
2012-04-01 19:24 Torr Samaho Fixed in Version => 1.0
2012-04-01 19:24 Torr Samaho Resolution open => fixed
2012-06-09 13:22 Torr Samaho Category General => Bug
2012-10-30 09:14 unknownna Relationship added related to 0000954
2018-09-30 19:52 Blzut3 Status resolved => closed




Questions or other issues? Contact Us.

Links

Copyright © 2000 - 2025 MantisBT Team
Powered by Mantis Bugtracker