Skip to content
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

Updating linter version to v1.41 #237

Merged
merged 3 commits into from
Jul 6, 2021

Conversation

armfazh
Copy link
Contributor

@armfazh armfazh commented Jun 29, 2021

Bumping up golangci-lint version to v1.41

  • replacing invocations of math/rand by crypto/rand

@armfazh armfazh added the maintenance Related to issues on the repository label Jun 29, 2021
@armfazh armfazh self-assigned this Jun 29, 2021
@armfazh armfazh changed the title Updating linter version to v.1.41 Updating linter version to v1.41 Jun 29, 2021
@cjpatton
Copy link
Contributor

* replacing invocations of math/rand by crypto/rand

Was this change made by the linter or did you make it manually?

@armfazh
Copy link
Contributor Author

armfazh commented Jun 30, 2021

* replacing invocations of math/rand by crypto/rand

Was this change made by the linter or did you make it manually?

I manually changed the instances reported by linter as insecure.

pke/kyber/internal/common/field_test.go Outdated Show resolved Hide resolved
pke/kyber/internal/common/field_test.go Outdated Show resolved Hide resolved
pke/kyber/internal/common/ntt_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@cjpatton cjpatton left a comment

Choose a reason for hiding this comment

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

Just some minor readability things.

for i := 0; i < N; i++ {
p[i] = int16(mathRand.Intn(18*int(Q) - 9*int(Q)))
p[i] = int16(int32(r[i] % max))
Copy link
Contributor

Choose a reason for hiding this comment

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

This change looks equivalent to me. However, the way the original code was written is weird. (Why 18 * int(Q) - 9 * int(Q) instead of just 9 * int(Q)?) This makes me think it might be worth double checking that this is the correct range.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bwesterb could you please give us some light here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's move this change forward, and do changes in other PR if bwesterb thinks it's needed.

Copy link
Member

@bwesterb bwesterb Jul 6, 2021

Choose a reason for hiding this comment

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

The intended code was int16(mathRand.Intn(9*int(Q)) - mathRand.Intn(18*int(Q))).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Bas, I will fix this on #240

pke/kyber/internal/common/poly_test.go Outdated Show resolved Hide resolved
sign/dilithium/internal/common/field_test.go Show resolved Hide resolved
pke/kyber/internal/common/field_test.go Outdated Show resolved Hide resolved
sign/dilithium/internal/common/pack_test.go Outdated Show resolved Hide resolved
@armfazh armfazh requested a review from cjpatton July 6, 2021 20:11
Copy link
Contributor

@cjpatton cjpatton left a comment

Choose a reason for hiding this comment

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

Approved, with a few suggestions.

pke/kyber/internal/common/poly_test.go Show resolved Hide resolved
sign/dilithium/internal/common/field_test.go Outdated Show resolved Hide resolved
pke/kyber/internal/common/field_test.go Outdated Show resolved Hide resolved
sign/dilithium/internal/common/field_test.go Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Related to issues on the repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants