Mailing-List: contact security-dev-help@xml.apache.org; run by ezmlm
Precedence: bulk
Reply-To: security-dev@xml.apache.org
MIME-Version: 1.0
Reply-To: coheigea@apache.org
In-Reply-To: <4C56D21A.20007@itumi.biz>
References: <4C56D21A.20007@itumi.biz>
Date: Tue, 3 Aug 2010 11:54:56 +0100
Message-ID: <AANLkTinf=9U6Bmha26+-ODi_OL5TnvpKpPiH_ZrpHtcp@mail.gmail.com>
Subject: Re: Status of == vs equals()
From: Colm O hEigeartaigh <coheigea@apache.org>
To: security-dev@xml.apache.org
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable

Hi Chad,

> My recommendation then is two fold:
>  - Ensure that nothing other than namespace bits are compared via =3D=3D.=
 I
> don't know that this occurs but the code should definitely be reviewed to
> ensure that.

Agreed.

>  - Create a new "NamespaceEqualityChecker" that provides methods for
> checking the various bits of a namespace (URIs, prefixes) and use it
> anywhere that either =3D=3D or equals() is used today.  Implementations b=
ased on
> =3D=3D and equals() would be provided with the default implementation bei=
ng
> equals()-based.  A configuration option should then be made available to
> control which impl gets used.

Sounds good. Have you thought about backwards compatibility issues
though, in relation to the ElementCheckerImpl? We don't want to break
backwards compatibility for a minor release if at all possible.

> I think that this should be addressed in the upcoming 1.4.4 release.  If
> quick consensus can be reached I'm willing to do the work with a window o=
f
> time I have available over the next 2-3 weeks.

That would be great! It would be interesting to run some benchmarks
when you're done to compare both options.

Colm.


On Mon, Aug 2, 2010 at 3:11 PM, Chad La Joie <lajoie@itumi.biz> wrote:
> So, while I don't have my access yet, Colm asked me if I'd take a look at
> the =3D=3D vs equals() issue (relevant bugs: 40897[1], 45637[2], 46681[3]=
)
>
> My executive summary is that clearly, as things stand, the current code
> favors optimization over correctness. =A0Rarely is this a good thing.
>
> Colm notes[4] that the reliance on intern'ed strings (and thus the abilit=
y
> to use =3D=3D) occurs sporadically throughout the code and not just withi=
n the
> ElementChecker implementations. =A0He specifically mentioned that the var=
ious
> C14N implementations, and indeed the =3D=3D is used about 6 times there f=
or
> string comparison.
>
> My recommendation then is two fold:
> =A0- Ensure that nothing other than namespace bits are compared via =3D=
=3D. I
> don't know that this occurs but the code should definitely be reviewed to
> ensure that.
>
> =A0- Create a new "NamespaceEqualityChecker" that provides methods for
> checking the various bits of a namespace (URIs, prefixes) and use it
> anywhere that either =3D=3D or equals() is used today. =A0Implementations=
 based on
> =3D=3D and equals() would be provided with the default implementation bei=
ng
> equals()-based. =A0A configuration option should then be made available t=
o
> control which impl gets used. =A0Additionally, it might even be possible =
to
> add some smarts that could detect known "good" parsers that use interning
> and automatically use the =3D=3D based implementation.
>
> I do not recommend changing any part of the code without addressing the
> whole codebase (i.e. all the =3D=3D's need to be fixed or no change shoul=
d be
> made) because of the possibility of creating new, unwanted, effects. =A0T=
he
> current functionality is undesirable but better the devil you know.
>
> I think that this should be addressed in the upcoming 1.4.4 release. =A0I=
f
> quick consensus can be reached I'm willing to do the work with a window o=
f
> time I have available over the next 2-3 weeks.
>
> [1] https://issues.apache.org/bugzilla/show_bug.cgi?id=3D40897
> [2] https://issues.apache.org/bugzilla/show_bug.cgi?id=3D45637
> [3] https://issues.apache.org/bugzilla/show_bug.cgi?id=3D46681
> [4] https://issues.apache.org/bugzilla/show_bug.cgi?id=3D45637#c1
> --
> Chad La Joie
> http://itumi.biz
> trusted identities, delivered
>