[Flightgear-devel] Buffer overflow in auto_gui.cxx
I got bitten by some static data corruption problems while working on
the panel stuff this afternoon (which I have almost working, by the
way -- expect a big code drop tomorrow morning). I tracked it down to
a buffer overflow in auto_gui.cxx. In the string formatting code for
the labels, there are some sprintf calls that look like this:
sprintf(buffer, "%05.2f", value);
Where the buffer is a static variable with a length of 8. It turned
out that one of the values was a huge garbage value -- something like
1e223.
This code looked correct to me, with the field width being less than
8. But it turns out that that C standard allows for the buffer to
overflow anyway. What happens is that the exponential form of the
number can't fit for whatever reason, so the glibc sprintf tries to
print it in (gack!) decimal. You can verify this with the following
tiny program:
int main() { printf("%05.2f\n", 1.1235e223); }
...which gives the following output on my machine:
11234993833496141165207167137504629455791386815894971093998449766224059134612227306117948559764428592704563810063396445147361721349723249504875046156126872109285397930933011042616316938278030005998645453598490624.00
Needless to say, the static data area wasn't happy with 200+ bytes of
overflow. :)
In my copy, I fixed this with snprintf, but something nags at me that
this isn't portable to some platform we care about. We could mock up
a slow version that did an unchecked sprintf into some very large
buffer and copied the result out. Probably a better idea is to sanify
the input values so they can't have such unprintable values.
Andy
--
Andrew J. RossNextBus Information Systems
Senior Software Engineer Emeryville, CA
[EMAIL PROTECTED] http://www.nextbus.com
"Men go crazy in conflagrations. They only get better one by one."
- Sting (misquoted)
___
Flightgear-devel mailing list
[EMAIL PROTECTED]
http://mail.flightgear.org/mailman/listinfo/flightgear-devel
Re: [Flightgear-devel] Buffer overflow in auto_gui.cxx
On Sun, 2002-06-23 at 21:14, Andy Ross wrote:
> I got bitten by some static data corruption problems while working on
> the panel stuff this afternoon (which I have almost working, by the
> way -- expect a big code drop tomorrow morning). I tracked it down to
> a buffer overflow in auto_gui.cxx. In the string formatting code for
> the labels, there are some sprintf calls that look like this:
>
> sprintf(buffer, "%05.2f", value);
>
> Where the buffer is a static variable with a length of 8. It turned
> out that one of the values was a huge garbage value -- something like
> 1e223.
>
> This code looked correct to me, with the field width being less than
> 8. But it turns out that that C standard allows for the buffer to
> overflow anyway. What happens is that the exponential form of the
> number can't fit for whatever reason, so the glibc sprintf tries to
> print it in (gack!) decimal. You can verify this with the following
> tiny program:
>
>int main() { printf("%05.2f\n", 1.1235e223); }
>
> ...which gives the following output on my machine:
>
>
>11234993833496141165207167137504629455791386815894971093998449766224059134612227306117948559764428592704563810063396445147361721349723249504875046156126872109285397930933011042616316938278030005998645453598490624.00
>
> Needless to say, the static data area wasn't happy with 200+ bytes of
> overflow. :)
>
> In my copy, I fixed this with snprintf, but something nags at me that
> this isn't portable to some platform we care about. We could mock up
> a slow version that did an unchecked sprintf into some very large
> buffer and copied the result out. Probably a better idea is to sanify
> the input values so they can't have such unprintable values.
Ahh, vindication is sweet!
At any rate, MSVC is at least one that has trouble with it, we use this
in JSBSim:
#ifdef _MSC_VER
#define snprintf _snprintf
#endif
>
> Andy
>
> --
> Andrew J. RossNextBus Information Systems
> Senior Software Engineer Emeryville, CA
> [EMAIL PROTECTED] http://www.nextbus.com
> "Men go crazy in conflagrations. They only get better one by one."
> - Sting (misquoted)
>
>
> ___
> Flightgear-devel mailing list
> [EMAIL PROTECTED]
> http://mail.flightgear.org/mailman/listinfo/flightgear-devel
--
Tony Peden
[EMAIL PROTECTED]
We all know Linux is great ... it does infinite loops in 5 seconds.
-- attributed to Linus Torvalds
___
Flightgear-devel mailing list
[EMAIL PROTECTED]
http://mail.flightgear.org/mailman/listinfo/flightgear-devel
RE: [Flightgear-devel] Buffer overflow in auto_gui.cxx
Andy Ross writes: > >I got bitten by some static data corruption problems > > sprintf(buffer, "%05.2f", value); > >In my copy, I fixed this with snprintf, but something nags at me that >this isn't portable to some platform we care about. #include should take care of snprintf portability problems BTW - good catch Norman ___ Flightgear-devel mailing list [EMAIL PROTECTED] http://mail.flightgear.org/mailman/listinfo/flightgear-devel
RE: [Flightgear-devel] Buffer overflow in auto_gui.cxx
Andy Ross writes:
>
>I got bitten by some static data corruption problems while working on
>the panel stuff this afternoon (which I have almost working, by the
>way -- expect a big code drop tomorrow morning). I tracked it down to
>a buffer overflow in auto_gui.cxx. In the string formatting code for
>the labels, there are some sprintf calls that look like this:
>
> sprintf(buffer, "%05.2f", value);
>
>Where the buffer is a static variable with a length of 8. It turned
>out that one of the values was a huge garbage value -- something like
>1e223.
>
>This code looked correct to me, with the field width being less than
>8. But it turns out that that C standard allows for the buffer to
>overflow anyway. What happens is that the exponential form of the
>number can't fit for whatever reason, so the glibc sprintf tries to
>print it in (gack!) decimal. You can verify this with the following
>tiny program:
>
> int main() { printf("%05.2f\n", 1.1235e223); }
>
>...which gives the following output on my machine:
>
>112349938334961411652071671375046294557913868158949
>710939984497662240591346122273061179485597644285927045638100633
>964451473617213497232495048750461561268721092853979309330110426
>16316938278030005998645453598490624.00
>
Note that getting a value outside of what is representable with %05.2f is
probably an indication of a bug elsewhere in the program as these represent
angular measurements only, where this construct is used in auto_gui.cxx
This could also be related to recent changes in the auto_pilot code to
run every thing through the 'properties' instead of accessing these values
directly where I believe they were always clamped to a +- 360 degree range
Norman
___
Flightgear-devel mailing list
[EMAIL PROTECTED]
http://mail.flightgear.org/mailman/listinfo/flightgear-devel
Re: [Flightgear-devel] Buffer overflow in auto_gui.cxx
Andy Ross <[EMAIL PROTECTED]> said: > Needless to say, the static data area wasn't happy with 200+ bytes of > overflow. :) > Hmmm...I wonder if this relates to some of the effects I've seen recently with the delta time and fdms. If you could put up a separate patch for this (before the "massive checkin" :-)), I'd like to run some tests to confirm. BTW, thanks for tracking this down. Best, Jim ___ Flightgear-devel mailing list [EMAIL PROTECTED] http://mail.flightgear.org/mailman/listinfo/flightgear-devel
Re: [Flightgear-devel] Buffer overflow in auto_gui.cxx
Jim Wilson wrote: > Hmmm...I wonder if this relates to some of the effects I've seen > recently with the delta time and fdms. If you could put up a > separate patch for this (before the "massive checkin" :-)), I'd like > to run some tests to confirm. BTW, thanks for tracking this down. This is what I did last night. It works for the bug I was seeing, but I didn't go through the code looking for other printf's that might blow up with the same values. The hard-coded "8" should be unified with the static definition of SliderText, of course. As Norman and I pointed out, though, a better solution would probably be to sanify the values before use if possible. Sometimes, if they come from arbitrary user input, you can't and have to resort to buffer size checking with snprintf and the like. But since these are angles, it should be possible. Andy RCS file: /var/cvs/FlightGear-0.7/FlightGear/src/Autopilot/auto_gui.cxx,v retrieving revision 1.29 diff -u -r1.29 auto_gui.cxx --- auto_gui.cxx26 Mar 2002 01:38:11 - 1.29 +++ auto_gui.cxx24 Jun 2002 17:04:59 - @@ -510,7 +510,7 @@ APAdjustHS0-> setCBMode ( PUSLIDER_DELTA ) ; APAdjustHS0-> setCallback ( maxroll_adj ) ; - sprintf( SliderText[ 0 ], "%05.2f", globals->get_autopilot()->get_MaxRoll() ); + snprintf( SliderText[ 0 ], 8, "%05.2f", globals->get_autopilot()->get_MaxRoll() ); APAdjustMaxRollTitle = new puText ( slider_title_x, slider_y ) ; APAdjustMaxRollTitle-> setDefaultValue ( "MaxRoll" ) ; APAdjustMaxRollTitle-> getDefaultValue ( &s ) ; @@ -527,7 +527,7 @@ APAdjustHS1-> setCBMode ( PUSLIDER_DELTA ) ; APAdjustHS1-> setCallback ( rollout_adj ) ; - sprintf( SliderText[ 1 ], "%05.2f", globals->get_autopilot()->get_RollOut() ); + snprintf( SliderText[ 1 ], 8, "%05.2f", globals->get_autopilot()->get_RollOut() ); APAdjustRollOutTitle = new puText ( slider_title_x, slider_y ) ; APAdjustRollOutTitle-> setDefaultValue ( "AdjustRollOut" ) ; APAdjustRollOutTitle-> getDefaultValue ( &s ) ; @@ -544,7 +544,7 @@ APAdjustHS2-> setCBMode ( PUSLIDER_DELTA ) ; APAdjustHS2-> setCallback ( rolloutsmooth_adj ) ; - sprintf( SliderText[ 2 ], "%5.2f", + snprintf( SliderText[ 2 ], 8, "%5.2f", globals->get_autopilot()->get_RollOutSmooth() ); APAdjustRollOutSmoothTitle = new puText ( slider_title_x, slider_y ) ; APAdjustRollOutSmoothTitle-> setDefaultValue ( "RollOutSmooth" ) ; @@ -562,7 +562,7 @@ APAdjustHS3-> setCBMode ( PUSLIDER_DELTA ) ; APAdjustHS3-> setCallback ( maxaileron_adj ) ; - sprintf( SliderText[ 3 ], "%05.2f", + snprintf( SliderText[ 3 ], 8, "%05.2f", globals->get_autopilot()->get_MaxAileron() ); APAdjustMaxAileronTitle = new puText ( slider_title_x, slider_y ) ; APAdjustMaxAileronTitle-> setDefaultValue ( "MaxAileron" ) ; -- Andrew J. RossNextBus Information Systems Senior Software Engineer Emeryville, CA [EMAIL PROTECTED] http://www.nextbus.com "Men go crazy in conflagrations. They only get better one by one." - Sting (misquoted) ___ Flightgear-devel mailing list [EMAIL PROTECTED] http://mail.flightgear.org/mailman/listinfo/flightgear-devel
Re: [Flightgear-devel] Buffer overflow in auto_gui.cxx
Andy Ross <[EMAIL PROTECTED]> said: > This is what I did last night. It works for the bug I was seeing, but > I didn't go through the code looking for other printf's that might > blow up with the same values. The hard-coded "8" should be unified > with the static definition of SliderText, of course. > > As Norman and I pointed out, though, a better solution would probably > be to sanify the values before use if possible. Sometimes, if they > come from arbitrary user input, you can't and have to resort to buffer > size checking with snprintf and the like. But since these are angles, > it should be possible. > That's basically what I did to get YASim models to load reliably on my system. I changed the FGInterface::_calc_timespan() (? guessing name from memory) so that it is sanifying delta_time to non-negative and under 10 seconds. But there's a bug somewhere that is causing these wild values to show up occasionally in the first place. I'm convinced that a couple of the reports we've had lately of trouble that miraculously goes away when some applies a seemingly unrelated CVS update are due to data corruption. Best, Jim ___ Flightgear-devel mailing list [EMAIL PROTECTED] http://mail.flightgear.org/mailman/listinfo/flightgear-devel
