patchGnash - The GNU Flash player - Patches: patch #7533, [PATCH] AbcBlock.h: checkBounds...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #7533: [PATCH] AbcBlock.h: checkBounds should check for negative values

Submitter:  Mj Mendoza IV <creek23>
Submitted:  Tue 26 Apr 2011 04:03:18 AM UTC
   
 
Category:  core Priority:  3 - Low
Status:  None Privacy:  Public
Assigned to:  None Open/Closed:  Open

Wed 27 Apr 2011 08:12:19 AM UTC, comment #3: 

The point is that if size_t can't be negative there's no point in checking for it to be < 0.
The compiler should notice that and warn you with something like: "comparison will always be false". Please check if it does.

If a caller tries to pass a negative number to a function expecting an unsigned type the body of the funcion will see that value reinterpreted as a positive value, which usually gets evaluated as > [maxvalue].

If a check has to be made for negative values it should be made outside of the function, that is while the value is still signed, right after reading it from the stack...

 Sandro Santilli <strk>
Group Member
Wed 27 Apr 2011 03:11:52 AM UTC, comment #2: 

I believe so, but I'm only trying to address a TODO inside `libcore/vm/Machine.cpp' at line 64:

<code>
/// TODO: it's quite possible for a malformed SWF to ask for out-of-bounds
/// pool access, although at the moment it's mainly Gnash bugs causing this.
/// Throwing an exception is good here, but it's not clear which one.
</code>

All `pool_{datatype}' are using `try/catch' to address the above TODO. But each `{datatype}PoolAt' are already using the checkBounds, which is not checking for negative values.

My suggestion is for `checkBounds' to implement checking of negative values so these try/catches will be removed.

 Mj Mendoza IV <creek23>
Tue 26 Apr 2011 10:08:40 AM UTC, comment #1: 

Isn't size_t unsigned ?
I'm pretty sure there's a ssize_t for signed type.

Sandro Santilli <strk>
Group Member
Tue 26 Apr 2011 04:03:18 AM UTC, original submission:  

There might be a chance that malformed values could be negative.

 Mj Mendoza IV <creek23>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #23304:  AbcBlock.h.patch added by creek23 (95B - application/octet-stream)
file #23305:  AbcBlock.h.patch.htm added by creek23 (209KiB - text/html)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by strk (Posted a comment)
  • -email is unavailable- added by creek23 (Submitted the item)

    •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2011-04-26 creek23 Attached File- Added AbcBlock.h.patch, #23304
        Attached File- Added AbcBlock.h.patch.htm, #23305

    Back to the top

    Copyright © 2024 Free Software Foundation, Inc.
    Verbatim copying and distribution of this entire article is permitted in any medium, provided this notice is preserved.
    The Levitating, Meditating, Flute-playing Gnu logo is a GNU GPL'ed image provided by the Nevrax Design Team.
    Page source code

    Powered by Savane 3.13-8a8d.
    Corresponding source code