Skip to content

Syncrepl fails to parse SyncInfoMessage when the message is a syncIdSet #351

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

Merged
merged 3 commits into from
Jun 18, 2020

Conversation

Firstyear
Copy link
Contributor

Previously, the SyncInfoMessage parser used a for loop to test for the
presence of each named choice with with the structure. However, because
refreshDelete and refreshPresent both are able to be fully resolved
as defaults, and due to how pyasn1 accesses named types, it was not
checking the choice tag, and would return a fully populated refreshDelete
to the caller instead.

This fixes the parser to always return the inner component, and retrieves
the name based on the current choice of the tagged item.

Author: William Brown william@blackhats.net.au

Previously, the SyncInfoMessage parser used a for loop to test for the
presence of each named choice with with the structure. However, because
refreshDelete and refreshPresent both are able to be fully resolved
as defaults, and due to how pyasn1 accesses named types, it was not
checking the choice tag, and would return a fully populated refreshDelete
to the caller instead.

This fixes the parser to always return the inner component, and retrieves
the name based on the current choice of the tagged item.

Author: William Brown <william@blackhats.net.au>
@codecov
Copy link

codecov bot commented Jun 8, 2020

Codecov Report

Merging #351 into master will increase coverage by 0.03%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master     #351      +/-   ##
==========================================
+ Coverage   71.30%   71.34%   +0.03%     
==========================================
  Files          50       50              
  Lines        4796     4795       -1     
  Branches      802      801       -1     
==========================================
+ Hits         3420     3421       +1     
+ Misses       1045     1043       -2     
  Partials      331      331              
Impacted Files Coverage Δ
Lib/ldap/syncrepl.py 63.18% <33.33%> (+0.89%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39ea8e5...544c99d. Read the comment docs.

@tiran tiran added this to the 3.3 milestone Jun 8, 2020
@tiran
Copy link
Member

tiran commented Jun 8, 2020

Thanks for the fix, William.

Do you have a reproducer bug for the test suite?

@encukou encukou removed this from the 3.3 milestone Jun 8, 2020
@Firstyear
Copy link
Contributor Author

Added a test case for you.

Copy link
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

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

Thanks for the test case!

After two espressos and a starring contest with ASN.1, I agree that the fix is correct. I merely propose to test the other attributes of the sync info message more thoroughly.

@tiran tiran removed the needs-tests label Jun 9, 2020
Good suggestion! Thanks!

Co-authored-by: Christian Heimes <christian@python.org>
@Firstyear
Copy link
Contributor Author

Thanks mate, appreciate the suggestion! I was a bit focused on the logic not the mesagge decode, but more coverage helps and helps guarantee correctness. Thanks!

@tiran
Copy link
Member

tiran commented Jun 9, 2020

@encukou This fix is a candidate for 3.3.0 release, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants