Hashing API

[unixninja92]

Here is the Hashing API I have written as part of my GSoC project. Also added a check of policy files for JCE in JCELoader.

[bertm]

[…] since last reset

I don't find a method that allows for resetting, nor one that specifies this as a side-effect.

[toad]

The finally block is wrong, isn't it? We don't want to recycle the digest if we have a successful construction?

[bertm]

Please add tests on cases that you expect to fail (using assertFalse and such, obviously) as well: you should have caught the problem in Hash.verify(…) that way.

[bertm]

Recycling a digest to which we still have references (namely in the member variabledigest) is bound to give problems at some point: another thread may obtain it by means of HashType.get() while we are still using it!

If you insist on recycling digests, I guess this could be done in the finalizer of Hash. Note that the semantics of finally don't have anything to do with finalizers!

[unixninja92]

I'm perfectly happy stripping the recycling code, I just included it as that is what we are currently doing.

[toad]

Where? The above is plainly wrong, is there similar code somewhere in fred at present? The problem here is we want to recycle it if it throws, but not otherwise. Actually we probably don't want to recycle it if it throws...

[bertm]

No, this is not what we are currently doing. Read src/freenet/crypt/MultiHashInputStream.java and src/freenet/crypt/MultiHashOutputStream.java, and closely watch the digest = null; immediately after the recycle.

[unixninja92]

I'll move this to some sort of close() method.

[unixninja92]

I'm trying to figure out the best way to use the AutoCloseable interface with Hash to fix the recycling problem. One idea is to make Hash an AutoCloseable that is interfaced by a different class that would use try-with-resources. Or I could push this on the user, but that doesn't feel like the right solution to me. My other idea is to write a MessageDigest that implements AutoCloseable that would be an interface to the underlying MessageDigests. I'm not sure which one would make the most sense though. Any thoughts/ideas?

[bertm]

My other idea is to write a MessageDigest that implements AutoCloseable that would be an interface to the underlying MessageDigests.

Something like the decorator pattern? Sounds good, but if we only use it in Hash, making Hash AutoClosable may make more sense.

[bertm]

Please be aware of the following: from MessageDigest Javadoc: "The digest method can be called once for a given number of updates. After digest has been called, the MessageDigest object is reset to its initialized state."
That may influence your choices in Javadoc writing. If getHash() really must reset the digest, please strongly emphasize this behaviour or even change the name of the method. Might intermediate hashes be considered useful, then you might want to clone the digest before invoking digest (if the digest implementation allows for this).

[bertm]

No need for throwing an exception since we can rest assured this ≠ otherObject holds when we arrive here, and thereby have a valid answer to return. The null case is comparable to !(otherObject instanceof HashResult) case (even better: !(null instanceof HashResult) always holds. so we can effectively entirely skip the null check here).

I don't think that I've ever come across an implementation that throws an NPE in equals, now I think of it.

return false; is more appropriate here, or even better, skip this entire check (the next check will do it for you).

[Thynix]

What's the different between byte[] and ByteBuffer? The former seems to be used in KeyGenUtils a lot, and the latter here. Why?

[Thynix]

Does Bouncy Castle have bugs filed for these? It may make sense to add tests to check that these are still broken, so that when / if those tests break the workaround can be removed.

Would this be more concise as a switch statement?

[unixninja92]

These are not BC classes. They are third party classes we are using.

[unixninja92]

IIRC all the issues @toad had with my other APIs was centered around how I was working with ByteBuffers. This API only has one method that uses ByteBuffers and it is only passing it on to a different method.

I've gone through all the comments on here, and fixed anything that hadn't already been fixed that should be (which was just rewriting a comment).

I've squashed everything down to one commit, and am working on writing up a nice commit message right now.

[unixninja92]

@Thynix Does this commit message meet the commit message standard, or should I add a newline between the hashing api and jceloader lines?

[Thynix]

It's close. The standards (won't be merged until at least someone else says they're happy with them - do they seem reasonable to you?) are here.

Looking at the standards, that message needs two changes for formatting:

• The summary needs to be imperative: "added" should be "add".
• The second line must be blank.

Other than that, this looks like it should be two commits (after all, it was trying to include two summaries) - one for adding the hashing API and another for the JCELoader changes. My initial reaction is that some parts of the commit body seem unnecessary to mention - this is intended to give relevant background information to someone reviewing the change when it is made, seeing the summary in the changelog, or looking back at it to get details on why the change was made and what it was intended to do.

Therefore I'd suggest something more like:

Add hashing API and tests

This makes it easier to use and change hashing algorithms.

and

JceLoader: log warning on limited policy file

Users with limited policy files should have an indication that
performance is known to be degraded and why.

Thoughts? Is this a reasonable response for me to make? I'd do something like this at my job but I'm having a lot of trouble figuring out what kind of feedback to give in open-source projects.

[Thynix]

It looks like this string was intended to also be printed to stdout / stderr? Otherwise I'd expect it to be within the logger call.

Longer-term I'm interested in Freenet moving to an existing logging framework and doing stdout/stderr with log level configuration instead of manually.

[unixninja92]

That makes sense. I think at the time I was just copying convention used at the file, but I'll change it.

[unixninja92]

@Thynix The policy seems reasonable to me.

Working on splitting the commit right now.

[unixninja92]

Should be good now. @Thynix should I merge it with master or should I leave that to you?

[Thynix]

Thanks! I'll have to make a 1468 release branch in order to merge this to next, but I plan to do so ... soon. Maybe tomorrow - my schedule is still fuzzy currently.

I haven't looked at the severity of the merge conflicts yet and might enlist your assistance in figuring them out if it's not obvious. If you're up for merging with next locally and fixing the conflicts that would be helpful.

[Thynix]

Can you remind me why this is a single Hash class that supports multiple hash types instead of an interface and multiple implementing classes? I might have asked this before but I haven't been able to find it. I ask this not so much to suggest a change but to document the reasoning.

[Thynix]

I get failing tests:

[junit] null
[junit] junit.framework.AssertionFailedError
[junit]     at freenet.crypt.HashResult.<init>(HashResult.java:30)
[junit]     at freenet.crypt.HashTest.testVerifyHashResultByteArrayWrongSizeMac(HashTest.java:333)

[Thynix]

I've merged this into a HashingAPI branch for inspection.

[bertm]

That assertion is expected to fail there. Consider the constructor code, and the name of the test where it fails.

public HashResult(HashType hashType, byte[] bs) {
    [...]
    assert(bs.length == type.hashLength);
}

We'd need to get rid of that assertion (at least for unit testing). Adding another (package-private) constructor without the assertion to accomplish this may be the easiest way out, without having to drop this assertion where it is useful.

@unixninja92 please update your configuration to enable assertions on unit testing.

[unixninja92]

@Thynix It's a single hash class to make it as easy as possible to upgrade to different hash types in the future.

I'm gonna try setting up my comp for freenet dev again and resolving the test issues. I think I did a fresh install since I last worked on this.

[unixninja92]

So I've fixed the assertion issue as @bertm suggested. I can't seem to figure out how to enable assertions for unit testing, so please let me know if there are any other places this needs to be fixed.
@Thynix I'll fix up commit messages after that has been tested.

[unixninja92]

@Thynix Fixed up the commits

[Thynix]

Thanks! 1471 is settling down for release, so we can look at merging this
after its release. 1471-pre3 - hopefully its last prerelease - is scheduled
for this weekend.

On Sun, Dec 27, 2015, 12:29 AM Charles Teese [email protected]
wrote:

@Thynix https://github.com/Thynix Fixed up the commits

—
Reply to this email directly or view it on GitHub
#258 (comment).

[xor-freenet]

@unixninja92

I can't seem to figure out how to enable assertions for unit testing, so please let me know if there are any other places this needs to be fixed.

They need to be enabled by passing -ea / -enableassertions to the JVM, and I would consider assertions being disabled in unit tests as a bug which should please be fixed or documented on the bugtracker before this is closed.

With the Ant builder it works like this.

Now that we've replaced Ant with Gradle on next it would be done like that:

tasks.withType(Test) {
    enableAssertions = true
}

Notice: It may be the default to enable them in Gradle as I obtained this code snipped from a stackexchange question where someone asked how to disable them.

If you want to increase the probability of this getting merged then please merge next backwards into this and apply the fix for Gradle if necessary instead of just fixing it for the Ant code you had based this upon.