620945 - CSSParserImpl::ParseDeclarationBlock() does not initialize changed

Status: RESOLVED INVALID

(Core :: CSS Parsing and Computation, defect)

Description

[timeless]

3528 CSSParserImpl::ParseDeclarationBlock(PRBool aCheckForBraces)

uninitialized:
3541       PRBool changed;
3542       if (!ParseDeclaration(declaration, aCheckForBraces,
passed referenced to changed:
3543                             PR_TRUE, &changed)) {

3933 	CSSParserImpl::ParseDeclaration(css::Declaration* aDeclaration,
3936 	                                PRBool* aChanged)
3937 	{

the next line is effectively: *aChanged = *aChanged | ...;
4038 	  *aChanged |= mData.TransferFromBlock(mTempData, propID,
4039 	                                       status == ePriority_Important,
4040 	                                       PR_FALSE, aMustCallValueAppended,
4041 	                                       aDeclaration);

Which means the undefined value from 3541 is mixed with the value from TransferFromBlock()

4042 	  return PR_TRUE;
4043 	}

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yes, but nothing ever reads the value, so why does it matter?

Comment 2

[:Gavin Sharp [email: [e-mail]]]

nsDOMCSSDeclaration::SetCssText seems to read the value (via ParseDeclarations), but it looks like the only difference in behavior that can cause is it calling SetCSSDeclaration() when things haven't actually changed.

Comment 3

[Boris Zbarsky [:bzbarsky]]

ParseDeclarations doesn't call ParseDeclarationBlock.  It has its own "changed" variable, and initializes it.

I agree with dbaron; we end up with a random value for a variable that nothing reads... which is ok.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

It calls ParseDeclaration(), which doesn't initialize aChanged. That's where the actual issue is, ParseDeclarationBlock is just one of the callers.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Nevermind, I missed the setting of aChanged in ParseDeclarations().

Comment 6

[Boris Zbarsky [:bzbarsky]]

This isn't a security issue.  Imo it's not an issue at all.