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.

Sign up for GitHub

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tests/Regress.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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!

hash ("hello world" :: Text) @=? 2668910425102664189
hashWithSalt (-3750763034362895579) ("hello world" :: Text) @=? 2668910425102664189
#endif
]
where
Expand Down