Skip to content

Fix test for -f random-initial-seed #220

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Fix test for -f random-initial-seed #220

wants to merge 1 commit into from

Conversation

jappeace
Copy link

This fixes the tests when running with:

cabal new-run hashable-tests -f random-initial-seed

I'd like to run these tests when patching hashable with nix.
By changing this into a hardcoded salt it works again.
I think this still guards against the regression
(although the test case isn't very descriptive)

This fixes the tests when running with:

    cabal new-run hashable-tests -f random-initial-seed

I'd like to run these tests when patching hashable with nix.
By changing this into a hardcoded salt it works again.
I think this still guards against the regression
(although the test case isn't very descriptive)
@@ -41,7 +41,7 @@ regressions = [] ++
hs @=? nub hs
#if WORD_SIZE_IN_BITS == 64
, testCase "64 bit Text" $ do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this test is at all in unordered-containers?

Copy link
Author

@jappeace jappeace Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is hashable on which unordered containers builds,
you've added this in august 0f8d8d1
I don't know what it does. (since it's in the Regress module, maybe there was a regression in the hash of text?).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I wonder what it was related to.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, now I remember. Well yes, I'm not "fixing" this. The test is there to ensure that we don't accidentally change the default salt, nor how Text is serialized - without reason.

random-initial-seed flag is there for testing only, and for testing of not hashable, so the test will stay unalrtered.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why I wouldn't want to change the default salt? It seems like a good workaround for that DoS vulnarbility CSSyd wrote about.

I was intending to do this for a company as future proving (no public aeson endpoints for now, so this isn't particularly urgent).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I don't consider random-initial-seed to be a security feature, it was added to aid testing, because many test-suites/doctests incorrectly assume the stability of hashable has functions, but it's still can be assumed that on a single system the hashable behaves deterministically.

See also commercialhaskell/stackage#5878

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, yeah I encountered xmlbf breaking as well.
I guess many people expect unordered containers to be ordered in their tests for some reason.

So I'll probably enable the flag for the company because it'd be pretty silly we got DoS'ed in the future. I'm aware of it now after all, and I'm afraid we'll forget. Or maybe there are additional reasons to not change the default salt?

Copy link
Contributor

@phadej phadej Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If case if you haven't followed, but there is a ongoing work on fixing this in aeson in more principled way, with first steps already merged haskell/aeson#866

So please calm down don't panic, especially as you don't have public aeson endpoints.

Copy link
Author

@jappeace jappeace Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't aware,
I think that's good enough for me to wait and see, thanks!

@phadej phadej closed this Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants