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 control packets being encoded when none exist on bind #142

Merged
merged 1 commit into from
Dec 9, 2017

Conversation

jefferai
Copy link
Contributor

@jefferai jefferai commented Dec 7, 2017

During the code reshuffle in #126 at bind time a call to encodeControls
was added. Unfortunately, the safety check around calling it when there
are no controls to encode were omitted from this call (unlike in del.go
and search.go). As a result the packet was always getting control
characters added even if there were none to encode.

I also modified the checks in del.go and search.go to be a little safer;
rather than check for a nil slice, it also will do the right thing when
the slice is not nil but there are no entries.

During the code reshuffle in go-ldap#126 at bind time a call to encodeControls
was added. Unfortunately, the safety check around calling it when there
are no controls to encode were omitted from this call (unlike in del.go
and search.go). As a result the packet was always getting control
characters added even if there were none to encode.

I also modified the checks in del.go and search.go to be a little safer;
rather than check for a nil slice, it also will do the right thing when
the slice is not nil but there are no entries.
Copy link
Member

@johnweldon johnweldon left a comment

Choose a reason for hiding this comment

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

Thank you!

@jefferai
Copy link
Contributor Author

jefferai commented Dec 9, 2017

Any ETA on when the merge button will be hit? :-)

@johnweldon johnweldon merged commit 23c4ee2 into go-ldap:master Dec 9, 2017
@jefferai
Copy link
Contributor Author

jefferai commented Dec 9, 2017

Thanks!

@johnweldon
Copy link
Member

I need to check in with @liggitt about the versioning on this, I think we can just tag it v2.5.2, but I'm a little hazy on the versioning process.

@jefferai
Copy link
Contributor Author

jefferai commented Dec 9, 2017

Sounds good. In the meantime I'm happy to vendor master.

@liggitt
Copy link
Contributor

liggitt commented Dec 13, 2017

I need to check in with @liggitt about the versioning on this, I think we can just tag it v2.5.2, but I'm a little hazy on the versioning process.

The referenced PR is in the master after the v2 branch was cut. If we want this in the v2 branch, we should cherry pick it

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.

3 participants