commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gary Gregory <garydgreg...@gmail.com>
Subject Re: [codec] Vacuous bit mask operation on integer value in UnixCrypt
Date Wed, 29 Aug 2012 13:05:56 GMT
How about this:

1. Let's get the digest package to 100% code coverage (or as closed to it
as possible)
2. Clean up the code (removing the "vacuous" code)

Also to do is fix the remaining check styles.

Patches welcome!

Gary

On Tue, Aug 28, 2012 at 2:47 AM, Thomas Neidhart
<thomas.neidhart@gmail.com>wrote:

> On 08/28/2012 03:33 AM, Christian Hammers wrote:
> > Am Tue, 28 Aug 2012 00:09:41 +0200
> > schrieb Christian Hammers <ch@lathspell.de>:
> >
> >> Hello
> >>
> >> Am Mon, 27 Aug 2012 16:39:23 -0400
> >> schrieb Gary Gregory <garydgregory@gmail.com>:
> >>
> >>> On Mon, Aug 27, 2012 at 2:53 PM, Thomas Neidhart
> >>> <thomas.neidhart@gmail.com>wrote:
> >>>
> >>>> On 08/27/2012 03:36 PM, Gary Gregory wrote:
> >>>>> Hi All:
> >>>>>
> >>>>> FindBugs says the following a couple of times for UnixCrypt:
> >>>>> "Vacuous bit mask operation on integer value
> >>>>> (INT_VACUOUS_BIT_OPERATION)
> >>>>>
> >>>>> This is an integer bit operation (and, or, or exclusive or) that
> >>>>> doesn't
> >>>> do
> >>>>> any useful work (e.g., v & 0xffffffff)."
> >>>>> This makes me wonder if the whole class is correct in the first
> >>>>> place and if/how/why these ops got in there.
> >>>>>
> >>>>> Was this translated (incorrectly) from JetSpeed's
> >>>>> implementation? Where
> >>>> the
> >>>>> types (int vs longs?) in JetSpeed different? Where did JetSpeed
> >>>>> get this implementation?
> >>>>
> >>>> I do not know the source of the JetSpeed implementation, but there
> >>>> are several (slightly different) variants of this floating around
> >>>> in other projects:
> >>>>
> >>>>
> >>>>
> http://grepcode.com/file/repository.jboss.org/maven2/postgresql/postgresql/8.4-701.jdbc3/org/postgresql/util/UnixCrypt.java
> >>>>
> >>>>
> >>>>
> http://grepcode.com/file/repo1.maven.org/maven2/org.mortbay.jetty/jetty/6.1.11/org/mortbay/jetty/security/UnixCrypt.jav
> >>>>
> >>>> The jetty version looks the most clean one, and as it is also
> >>>> Apache license, maybe we should take this one and cleanup the
> >>>> code a bit?
> >>>>
> >>>
> >>> The first thing to try would be to drop in the Jetty impl (I looked
> >>> at the one in the 8.1 version) and see our unit tests run unchanged
> >>> (aside from the different APIs). I cannot do this today though. Feel
> >>> try to try ;)
> >>>
> >>> Gary
> >>
> >> The original Jetspeed source also had this "v & 0xffffffff":
> >>
> http://www.javadocexamples.com/java_source/org/apache/jetspeed/services/security/ldap/UnixCrypt.java.html
> >>
> http://archive.apache.org/dist/jakarta/jetspeed/jetspeed-current-src.zip
> >>
> >> But I will have a look now how the Jetty source looks in comparison...
> >
> > The Aki Yoshida version from Jetty does not pass the UTF-8 unit tests:
> > It gives the same result for ASCII inputs and also for a single 0x80
> > but two consecutive bytes > 0x80 like 0xC3 0xA4 for "ä" gives a
> > different result than "perl -e 'print crypt("ä", "xx");'.
> > As it looks totally different than all the other UnixCrypt
> > implementations I have no clue how to fix it.
> >
> > Regarding the other ones, most of them have a common ancestors in
> > John Dumas Java port which is the one with the "v & 0xffffffff".
> > http://www.dynamic.net.au/christos/crypt/ gives a nice
> > genealogy overview :)
> >
> > The first one mentioned there, from Davit Scott, also works but is
> > not only significantly slower than all the other ones but also
> > does not have a clear license statement so I doubt that we could
> > use it.
> >
> > Our current version, which is similar to the PostgreSQL JDBC one
> > mentioned above, passes all tests without the "0xffffffff" though.
> > As it "int" is defined as a 4 bytes in the JLS 4.2.1 and not platform
> > dependend like in C, we could just remove it and make FindBugs happy.
> >
> > Apropos, "Math.abs(r.nextInt()) % numSaltChars" could be replaced by
> > "r.nextInt(numSaltChars)" and FindBugs would be completely
> > clean for this file.
>
> Interesting is also that in the postgresql variant, the bitwise and
> operation is using a slightly different argument:
>
>  c &= 0x0fffffff;
>
> So they must have made this observation too because the rest of the code
> looks very much the same.
>
> Thomas
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>


-- 
E-Mail: garydgregory@gmail.com | ggregory@apache.org
JUnit in Action, 2nd Ed: <http://goog_1249600977>http://bit.ly/ECvg0
Spring Batch in Action: <http://s.apache.org/HOq>http://bit.ly/bqpbCK
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message