bugfix: Fix global-buffer-overflow in WorldBuilder (MapObjectProps class) #1725
Conversation
xezon
added
Major
xezon
force-pushed
the
xezon/fix-worldbuilder-mapobjects-crash
branch
from
15d476a to
0455798
Compare
Skyaero42
left a comment
Skyaero42
left a comment
There was a problem hiding this comment.
I'm not convinced all size changes are necessary.
| // CWorldBuilderDoc* pDoc = CWorldBuilderDoc::GetActiveDoc(); | ||
| CWnd* edit; | ||
| static char buff[12]; | ||
| static char buff[64]; |
There was a problem hiding this comment.
I asked Chat and it recommended 44. So I picked 64 for floats.
Chat
Good — this one’s much simpler and we can calculate it exactly.
You said you’re using sprintf with a format of "%.2f", and printing a float value (e.g. 0.2f).
Let’s break it down precisely 👇
🧮 Format "%.2f"
-
%.2fmeans:-
Always print 2 digits after the decimal point
-
No exponential notation — just fixed‐point
-
A leading digit before the decimal (even if the number is < 1)
-
A sign (
-) if the value is negative
-
📏 Maximum Possible String Length
For a float, the largest finite value is about 3.4028235e+38.
When formatted with "%.2f", this becomes an enormous number of digits — because %f prints the full decimal expansion, not scientific notation.
That means:
3.4028235e+38 ≈ 340282350000000000000000000000000000000.00
That’s 39 digits before the decimal point, plus:
-
1 for the decimal point
-
2 for the fractional digits
-
1 for possible negative sign
-
1 for the null terminator
So total = 39 + 1 + 2 + 1 + 1 = 44 bytes
✅ Safe Buffer Size
If you plan to sprintf(buf, "%.2f", someFloat);
you should allocate at least 44 bytes to safely hold any float value.
To be nice and round, use char buf[48]; or char buf[64]; — it costs nothing extra and covers all cases cleanly.
🧠 Example
char buf[48];
float x = 0.2f;
sprintf(buf, "%.2f", x); // produces "0.20"
In this specific example, "0.20" takes 4 characters, so even char buf[8]; would work —
but to be safe for all float values, you want ~44 bytes minimum.
TL;DR
| Case | Example Output | Needed Size |
|---|---|---|
| Typical value (e.g. 0.2f) | "0.20" | 5 bytes |
| Maximum float value | "340282346638528859811704183484516925440.00" | 44 bytes |
| ✅ Safe general size | 48 bytes |
Would you like me to show a small C snippet that actually computes the worst-case string length automatically (so you can test it on your compiler)?
There was a problem hiding this comment.
how likely is it that m_angle or m_height has a value this size?
There was a problem hiding this comment.
Very unlikely. What size do you want for floats then?
There was a problem hiding this comment.
For height and angle, I think 12 is enough. 16 if you want multiples of 2
There was a problem hiding this comment.
How about we do 32 for floats, 12 for 4 byte decimals?
And in another change we change the static char to char. There is no need for these buffers to be in data segment.
There was a problem hiding this comment.
Float string buffers decreased from 64 to 32.
| if (pItem) { | ||
| static char buff[12]; | ||
| sprintf(buff, "%g", stoppingDistance); | ||
| static char buff[64]; |
There was a problem hiding this comment.
this one is correct. scientific notation needs at least 14 characters, so 12 is not enough. 64 may be overkill.
| { | ||
| char buffer[50]; | ||
| sprintf(buffer, "%.2f", height); | ||
| char buffer[64]; |
| if (m_lightFeedbackMesh[lIndex] == NULL) | ||
| { char nameBuf[64]; | ||
| sprintf(nameBuf,"WB_LIGHT%d",lIndex+1); | ||
| snprintf(nameBuf,ARRAY_SIZE(nameBuf),"WB_LIGHT%d",lIndex+1); |
There was a problem hiding this comment.
I omitted space because it did not have space originally.
There was a problem hiding this comment.
Maybe take this opportunity to adjust it? That's what I did with strlcpy.
| m_position = *pMapObj->getLocation(); | ||
| static char buff[12]; | ||
| sprintf(buff, "%0.2f, %0.2f", m_position.x, m_position.y); | ||
| static char buff[128]; |
|
|
||
| CWnd* edit; | ||
| static char buff[36]; | ||
| static char buff[64]; |
xezon
force-pushed
the
xezon/fix-worldbuilder-mapobjects-crash
branch
from
33b600e to
284142e
Compare
|
Replicated in Generals with conflicts |
2 participants
This change fixes global-buffer-overflow's in WorldBuilder, particularly in the
MapObjectPropsclass.To mitigate, select buffer sizes were increased (sometimes decreased when so ok). Additionally, all
sprintfcalls in WorldBuilder have been changed tosnprintffor additional robustness.TODO