Crash [@ nsContextBoxBlur::BoxBlurVertical] with text-shadow

RESOLVED FIXED

Status

()

Core
Layout
--
critical
RESOLVED FIXED
9 years ago
2 years ago

People

(Reporter: Jesse Ruderman, Assigned: Michael Ventnor)

Tracking

(Blocks: 1 bug, {crash, testcase})

Trunk
x86
Mac OS X
crash, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?], crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
Created attachment 332091 [details]
testcase

Loading the testcase makes nsContextBoxBlur::BoxBlurVertical dereference a random address, which usually causes a crash.
(Reporter)

Updated

9 years ago
Whiteboard: [sg:critical?]
(Assignee)

Comment 1

9 years ago
No crash here. OS/Build?

Is your build up-to-date? I remember fixing a crash in BoxBlurVertical before that Martijn found.
(Assignee)

Comment 2

9 years ago
Created attachment 332101 [details] [diff] [review]
Patch

Never mind; its random but I could eventually reproduce this. I get ridiculous values for y for the x[y] operator, but what seemed suspicious was that I was getting frames with height 1. This might explain why you're crashing in vertical blur which is after a perfectly fine horizontal blur.

You can't average 1 value and get anything different, so the easiest thing to do is return early if you only get one pixel in the given direction.

Lets hope I'm right. Can you try again with this patch?
Assignee: nobody → ventnor.bugzilla
Status: NEW → ASSIGNED
Attachment #332101 - Flags: superreview?(roc)
Attachment #332101 - Flags: review?(roc)
(Reporter)

Comment 3

9 years ago
I think the problem is this line in BoxBlurVertical:

pos = PR_MIN(pos, stride - 1);

That constraint makes sense inBoxBlurHorizontal, but in BoxBlurVertical I think pos needs to be restricted by the number of rows instead.
(Assignee)

Comment 4

9 years ago
(In reply to comment #3)
> I think the problem is this line in BoxBlurVertical:
> 
> pos = PR_MIN(pos, stride - 1);
> 
> That constraint makes sense inBoxBlurHorizontal, but in BoxBlurVertical I think
> pos needs to be restricted by the number of rows instead.
> 

No, rows is not used in that block but stride is.
(Reporter)

Comment 5

9 years ago
The reason I'm suspicious of "pos = PR_MIN(pos, stride - 1);" is that pos is about to be *multiplied* by stride.
(Assignee)

Comment 6

9 years ago
Now that I look at it, you're probably right. I wonder why it worked before then...
(Assignee)

Comment 7

9 years ago
Created attachment 332108 [details] [diff] [review]
Patch 2

Yeah, that is the cause because I re-read the old blurring code in SVG that I copied this from originally.
Attachment #332101 - Attachment is obsolete: true
Attachment #332108 - Flags: superreview?(roc)
Attachment #332108 - Flags: review?(roc)
Attachment #332101 - Flags: superreview?(roc)
Attachment #332101 - Flags: review?(roc)
(Reporter)

Comment 8

9 years ago
Patch 2 fixes the bug for me :)
Attachment #332108 - Flags: superreview?(roc)
Attachment #332108 - Flags: superreview+
Attachment #332108 - Flags: review?(roc)
Attachment #332108 - Flags: review+
(Assignee)

Comment 9

9 years ago
roc or jesse, can you push this for me?
Keywords: checkin-needed
Pushed 6524c183be7a. I added Jesse's test as a crashtest --- Michael, it would be slightly more convenient if you added that in your patch next time.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Keywords: checkin-needed
(Reporter)

Comment 11

9 years ago
Is it possible to test this without gigantic widths and heights?
Crash Signature: [@ nsContextBoxBlur::BoxBlurVertical]

Updated

2 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.