Christian Holler (:decoder) Reporter
	Description • 11 years ago

Right now, we have many occurrences of signed integer overflow in our codebase, that are related to nscoord. Since signed integer overflow is undefined behavior, the compiler can make possibly dangerous optimizations based on the assumption that no overflows can occur (including removing overflow checks/handling). In addition, several of our tests show that at runtime we are actually overflowing in those places. Finally, fixing these issues is a prerequisite to allow further fuzzing in for this type of problem and to find (possibly more critical) other issues as well.

	Jesse Ruderman
	Comment 1 • 11 years ago

If we don't care what happens on overflow except to want the result to be defined, how about creating a "int with uint-like wrapping" type?  With operator overloading and inlining, it should feel and perform just like int does under -fwrapv.

	Christian Holler (:decoder) Reporter
	Comment 2 • 11 years ago

(In reply to Jesse Ruderman from comment #1)
> If we don't care what happens on overflow except to want the result to be
> defined, how about creating a "int with uint-like wrapping" type?  With
> operator overloading and inlining, it should feel and perform just like int
> does under -fwrapv.

Sounds like a good approach to me :) Putting needinfo on Mats to get his opinion on it and see how we can proceed there.

	Mats Palmgren (inactive)
	Comment 3 • 11 years ago

I responded in bug 919486... I'm still unconvinced that this is a problem we need
to fix for security reasons.

	Christian Holler (:decoder) Reporter
	Comment 4 • 11 years ago

(In reply to Mats Palmgren (:mats) from comment #3)
> I responded in bug 919486... I'm still unconvinced that this is a problem we need to fix for security reasons.

Are you convinced that it is problematic at all? Would you be ok with some sort of wrapper? Maybe such a wrapper would making the switch between int and float also easier, and one could implement whatever clamping/overflowing logic more easily. I would try to write it even :)

	Mats Palmgren (inactive)
	Comment 5 • 11 years ago

> Are you convinced that it is problematic at all?

I see two problems:
1. it spams your fuzzing test with nscoord overflows which makes real
   problems harder to detect
2. unexpected layout when an overflow does occur

1 can be solved by using NS_COORD_IS_FLOAT for this test.
2 is very rarely a problem affecting any real web sites.

> Would you be ok with some sort of wrapper?

I'm not against adding a wrapper per se, but I don't think adding a run-time
cost in release builds to solve the above problems is justified.

	Christian Holler (:decoder) Reporter
	Comment 6 • 11 years ago

(In reply to Mats Palmgren (:mats) from comment #5)

> 
> 1 can be solved by using NS_COORD_IS_FLOAT for this test.

Unfortunately that doesn't compile. While writing the wrapper I also found a few places in the code where I think that replacing nscoord with float will just break, but I'd have to take a closer look on that, I could be wrong.

> 2 is very rarely a problem affecting any real web sites.

If it would just be wrong layout, sure. But can we be sure about that? I can hardly imagine it's possible to keep an eye on every single thing the compiler could optimize there that would lead to other failures, but then again you know the code much better than I do :)

> 
> > Would you be ok with some sort of wrapper?
> 
> I'm not against adding a wrapper per se, but I don't think adding a run-time
> cost in release builds to solve the above problems is justified.

Okay. I don't know how high the performance impact would actually be, maybe it is marginal and could also help with other stuff (like maintaining float better).

	Mats Palmgren (inactive)
	Comment 7 • 11 years ago

(In reply to Christian Holler (:decoder) from comment #6)
> Unfortunately that doesn't compile.

We should fix that, please file bugs.

> Okay. I don't know how high the performance impact would actually be, maybe
> it is marginal and could also help with other stuff (like maintaining float
> better).

I don't think even a marginal performance impact is acceptable since this
really isn't a problem we need to fix in the first place.

	Jesse Ruderman
	Comment 8 • 11 years ago

The performance impact in opt builds should be exactly the cost of NOT silently removing overflow checks that are part of the code.

	u483800
	Comment 9 • 11 years ago

Oh, I have no problem with that of course.  Sorry, I thought you were suggesting making
all nscoord arithmetic operations *saturating* by default.  I must have mixed this up
with the discussion on some other bug.

	Christian Holler (:decoder) Reporter
	Comment 10 • 11 years ago

I don't know what saturating means in that context, I'm just trying to make the behavior defined :)

@mats: If you are in Brussels by any chance, let's find a time to chat a bit about this.

	Mats Palmgren (inactive)
	Comment 11 • 11 years ago

https://en.wikipedia.org/wiki/Saturation_arithmetic

I thought you were suggesting we should implement that for all nscoord operations so 
that the result would always stay in the interval nscoord_MIN .. nscoord_MAX without
overflowing, e.g. doing all operations using 64-bit values and then capping it or some
such.  It seems I misunderstood.  I have no problem with doing some casts or whatever
just to make the over/underflow defined.  As Jesse said that should not incur any
runtime cost other than preventing some unfortunate optimizations by the compiler.

Sure, I'm in Brussels.

	Jesse Ruderman
	Comment 12 • 10 years ago

If we want to do both this and bug 765861, http://blog.regehr.org/archives/1139 has tips on how to get both well-defined wrapping behavior and overflow checks.