Bug 84952 - [PATCH] Recording surface ink extents: difference between two fixed point numbers may overflow before conversion to double
Summary: [PATCH] Recording surface ink extents: difference between two fixed point num...
Status: RESOLVED MOVED
Alias: None
Product: cairo
Classification: Unclassified
Component: general (show other bugs)
Version: 1.12.16
Hardware: All All
: medium normal
Assignee: Chris Wilson
QA Contact: cairo-bugs mailing list
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-13 06:35 UTC by Carlos Antunes
Modified: 2018-08-25 13:46 UTC (History)
0 users

See Also:
i915 platform:
i915 features:

Attachments
Patch fixes fixed-point overflow bug on recording-surface (594 bytes, text/plain)
2014-10-13 06:35 UTC, Carlos Antunes 		Details
Simple test to exercise bug (651 bytes, text/plain)
2014-10-13 17:32 UTC, Carlos Antunes 		Details
View All

Description Carlos Antunes 2014-10-13 06:35:39 UTC 
Created attachment 107759 [details]
Patch fixes fixed-point overflow bug on recording-surface

Recording surface may return incorrect result for ink extents when, for example, it is created unbounded. This is due to a bug in the conversion of a difference from fixed to floating point. The code for cairo_recording_surface_ink_extents reads at some point:

*width = _cairo_fixed_to_double (bbox.p2.x - bbox.p1.x);
*height = _cairo_fixed_to_double (bbox.p2.y - bbox.p1.y);

The "solution" (short of getting rid of fixed point altogether) is to change this to:

*width = _cairo_fixed_to_double (bbox.p2.x) - _cairo_fixed_to_double (bbox.p1.x);
*height = _cairo_fixed_to_double (bbox.p2.y) - _cairo_fixed_to_double (bbox.p1.y);

This was tested on MSYS2/MinGW64 and indeed it works as expected.

Attached, you'll find the simple patch. It works against 1.12.16 but the bug is still present on HEAD.

Thanks!
Comment 1 Uli Schlachter 2014-10-13 06:57:02 UTC 
Do you happen to have a self-contained example program demonstrating the problem that you are fixing? Could you provide patches as git commits (google for git format-patch).

I am not sure about the problem that you are fixing. If the extents are large enough to overflow the fixed point calculations, wouldn't other code (which also uses the fixed point numbers) possibly run into the same overflows? So while I agree that fixing this is a good idea, I am not sure that this actually means that it becomes safe to use a recording surface with large-ish coordinates.
Comment 2 Carlos Antunes 2014-10-13 17:32:30 UTC 
Created attachment 107786 [details]
Simple test to exercise bug
Comment 3 Carlos Antunes 2016-12-15 18:57:12 UTC 
It's amazing that, after more than two years, this trivial to fix bug is still present in the code!
Comment 4 GitLab Migration User 2018-08-25 13:46:32 UTC 
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/cairo/cairo/issues/188.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.