Closed Bug 1035168 Opened 6 years ago Closed 6 years ago

Nightly crash in _VEC_memcpy | mozilla::gfx::CopyToImageSurface with address 0x0

Categories

(Core :: Graphics, defect, critical)

33 Branch
x86
Windows NT
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla33
Tracking Status
firefox31 --- unaffected
firefox32 + verified
firefox33 --- verified

People

(Reporter: kairo, Assigned: mattwoodrow)

Details

(Keywords: crash)

Crash Data

Attachments

(2 files)

This bug was filed from the Socorro interface and is 
report bp-a031b3b5-2db3-41ee-8053-61adb2140707.
=============================================================

0 	msvcr100.dll 	_VEC_memcpy 	
1 	xul.dll 	mozilla::gfx::CopyToImageSurface(unsigned char *,mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const &,int,mozilla::gfx::SurfaceFormat) 	gfx/2d/DrawTargetCairo.cpp
2 	xul.dll 	mozilla::gfx::GetCairoSurfaceForSourceSurface(mozilla::gfx::SourceSurface *,bool) 	gfx/2d/DrawTargetCairo.cpp
3 	xul.dll 	mozilla::gfx::DrawTargetCairo::DrawSurface(mozilla::gfx::SourceSurface *,mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> const &,mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> const &,mozilla::gfx::DrawSurfaceOptions const &,mozilla::gfx::DrawOptions const &) 	gfx/2d/DrawTargetCairo.cpp
4 	xul.dll 	imgFrame::LockImageData() 	image/src/imgFrame.cpp
5 	xul.dll 	mozilla::image::RasterImage::DrawWithPreDownscaleIfNeeded(imgFrame *,gfxContext *,GraphicsFilter,gfxMatrix const &,gfxRect const &,nsIntRect const &,unsigned int) 	image/src/RasterImage.cpp
[...]

I saw a case with a somewhat different stack but the same few top frames, see bp-0f9cd839-d27f-4f86-a204-39c0a2140707

The address of all those crashes is 0x0, so it smells like a null pointer deref to me. The vast majority of those crashes is on Win7, but some are 8.1 or Vista. Also, the vast majority of those crashes are on Intel graphics, a very few on AMD. URLs are all over the place, with Facebook as the usual suspect on top.

More details and crashes can be found at https://crash-stats.mozilla.com/report/list?signature=_VEC_memcpy%20%7C%20mozilla%3A%3Agfx%3A%3ACopyToImageSurface%28unsigned%20char%2A%2C%20mozilla%3A%3Agfx%3A%3AIntSizeTyped%3Cmozilla%3A%3Agfx%3A%3AUnknownUnits%3E%20const%26%2C%20int%2C%20mozilla%3A%3Agfx%3A%3ASurfaceFormat%29

This started happening with the 20140702030201 build on Nightly, so the regression range seems to be http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=83c09fe3a658&tochange=7075808c3306
https://crash-stats.mozilla.com/report/list?signature=memcpy%20%7C%20mozilla%3A%3Agfx%3A%3ACopyToImageSurface%28unsigned%20char%2A%2C%20mozilla%3A%3Agfx%3A%3AIntSizeTyped%3Cmozilla%3A%3Agfx%3A%3AUnknownUnits%3E%20const%26%2C%20int%2C%20mozilla%3A%3Agfx%3A%3ASurfaceFormat%29 seems to be the same issue.
Crash Signature: [@ _VEC_memcpy | mozilla::gfx::CopyToImageSurface(unsigned char*, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, int, mozilla::gfx::SurfaceFormat)] → [@ _VEC_memcpy | mozilla::gfx::CopyToImageSurface(unsigned char*, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, int, mozilla::gfx::SurfaceFormat)] [@ memcpy | mozilla::gfx::CopyToImageSurface(unsigned char*, mozilla::gfx::IntSizeTyped<mo…
It may have been: 	
db4f6e215872	Matt Woodrow — Bug 997304 - Copy the image data if it's not a suitable size for cairo. r=Bas

Let me know if I'm reading this correctly:
http://hg.mozilla.org/mozilla-central/rev/db4f6e215872#l1.58

If there is an invalid stride and data->GetData() is null, then we do CopyToImageSurface and the memcpy crashes with a null src. Does that seem plausible?
Flags: needinfo?(matt.woodrow)
Yeah, sounds plausible. It looks like cairo verifies the stride (and returns the CAIRO_STATUS_INVALID_STRIDE error surface) before checking if the data parameter is non-null. A stride of 0 (which is what I'd expect if GetData returns nullptr) would have this effect.
Flags: needinfo?(matt.woodrow)
Assignee: nobody → matt.woodrow
Attachment #8452106 - Flags: review?(bas)
Attachment #8452106 - Flags: review?(bas) → review+
This got backed out because of win7 assertions. SourceSurfaceD2D asserts if we try mix usage of GetData() and Map(), which happens when FilterNodeSoftware checks the stride before calling DrawSurface.
Attachment #8455094 - Flags: review?(bas)
Comment on attachment 8455094 [details] [diff] [review]
Avoid calling GetData/Stride on a surface that we will later Map.

Review of attachment 8455094 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/FilterNodeSoftware.cpp
@@ +820,5 @@
> +    DataSourceSurface::MappedSurface map;
> +    if (result->Map(DataSourceSurface::READ, &map)) {
> +       // Unmap immediately since CloneAligned hasn't been updated
> +       // to use the Map API yet. We can still read the stride/data
> +       // values as long as we don't try to dereference them.

nit: Add a todo here, this is pretty bad, as there's technically no guarantee a subsequent map will have the same stride.
Attachment #8455094 - Flags: review?(bas) → review+
https://hg.mozilla.org/mozilla-central/rev/524dc207d68f
https://hg.mozilla.org/mozilla-central/rev/45e32200a0c4
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Hrm, and just as we fixed this, bug 997304 was uplifted to 32 without uplifting this one as well. :(
Matt - Given that this impacts 32, can we nom for beta uplift?
Flags: needinfo?(matt.woodrow)
Comment on attachment 8452106 [details] [diff] [review]
Check that the data is available

Approval Request Comment
[Feature/regressing bug #]: Bug Bug 997304
[User impact if declined]: Occasional crashes, unsure of the frequency.
[Describe test coverage new/current, TBPL]: Confirmed that crash reports stopped being filed
[Risks and why]: Very low risk, basically just a null check and a small change to avoid a deprecated API (since it caused assertions).
[String/UUID change made/needed]: None
Attachment #8452106 - Flags: approval-mozilla-beta?
Flags: needinfo?(matt.woodrow)
Comment on attachment 8452106 [details] [diff] [review]
Check that the data is available

beta+
Attachment #8452106 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Looking through Socorro, I can see the following:

1. [@ _VEC_memcpy | mozilla::gfx::CopyToImageSurface(unsigned char*, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, int, mozilla::gfx::SurfaceFormat)]
- there are NO more crashes in builds after July 22nd (last build with this crash was 32.0, BuildID=20140722195627)

2. [@ memcpy | mozilla::gfx::CopyToImageSurface(unsigned char*, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, int, mozilla::gfx::SurfaceFormat)] - there are 2 crashes in builds older than July 22nd
- Firefox 32 - 1 crash in Beta 5 build with ID=20140807212602
- Firefox 34 - 1 crash in Nightly build with ID=20140804030205

Given the very low occurrence in Builds after July 22nd, I'm closing this issue.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.