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

fix: explicitly use uint64 for payload length #1949

Merged
merged 2 commits into from
Jun 2, 2021
Merged

fix: explicitly use uint64 for payload length #1949

merged 2 commits into from
Jun 2, 2021

Conversation

dnwe
Copy link
Collaborator

@dnwe dnwe commented May 20, 2021

Tested with:

docker run --platform=linux/i386 --rm -v "$PWD":/usr/src -w /usr/src golang:1.16 go build -v

Fixes #1943

Tested with:
```
docker run --platform=linux/i386 --rm -v "$PWD":/usr/src -w /usr/src golang:1.16 go build -v
```

Fixes #1943
@dnwe dnwe requested a review from bai as a code owner May 20, 2021 23:01
@ghost ghost added the cla-needed label May 20, 2021
@dnwe dnwe mentioned this pull request May 20, 2021
@@ -57,7 +57,7 @@ type KerberosClient interface {

// writePackage appends length in big endian before the payload, and sends it to kafka
func (krbAuth *GSSAPIKerberosAuth) writePackage(broker *Broker, payload []byte) (int, error) {
length := len(payload)
length := uint64(len(payload))
size := length + 4 // 4 byte length header + payload
if size > math.MaxUint32 {
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to change this to size > math.MaxInt32 rather than MaxUint32. Line 65 would panic on a 32-bit system if size is > math.MaxInt32. But, you are kinda asking for it if you are trying to write > 2GB of data all at once.

Copy link
Contributor

Choose a reason for hiding this comment

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

I signed the CLA, btw, but can't comment on the PR for some reason. (Not sure I really needed to be the owner of such a small change :D, but thanks for including me.)

@ghost ghost removed the cla-needed label Jun 1, 2021
@dnwe dnwe merged commit fca0631 into master Jun 2, 2021
@bai bai deleted the dnwe/32bit branch September 13, 2021 07:01
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.

Can't build in 32-bit
2 participants