Skip to content

Conversation

adhintz
Copy link
Contributor

@adhintz adhintz commented Jul 21, 2020

This is to fix the can't pickle thread.lock objects error when attempting to pickle something such as an ecdsa.keys.VerifyingKey

Alternatively, the custom getstate and setstate could be within the RWLock class.

@coveralls
Copy link

coveralls commented Jul 21, 2020

Coverage Status

Coverage increased (+0.01%) to 96.988% when pulling e0840fa on adhintz:master into a776084 on warner:master.

Copy link
Member

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

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

Could you add a test case to verify that the object is pickleable? and that unpickling gives an equal object?

@tomato42
Copy link
Member

also black seems to complain about formatting, please fix that too

@adhintz
Copy link
Contributor Author

adhintz commented Jul 22, 2020

Thanks for the response! While writing the test, I cannot get equality comparisons working for PointJacobi objects. Debugging __eq__ in PointJacobi, it's deciding two PointJacobi objects with the same values are not equal because of if self.__curve != other.curve(): Although class CurveFp defines __eq__, it appears to be doing an identity comparison and not using the CurveFp __eq__. Here's an example test case:

self = <ecdsa.test_jacobi.TestJacobi testMethod=test_can_pickle>

    def test_can_pickle(self):
        pj1 = PointJacobi(curve=CurveFp(23, 1, 1, 1), x=2, y=3, z=1, order=1)
        pj2 = PointJacobi(curve=CurveFp(23, 1, 1, 1), x=2, y=3, z=1, order=1)
>       self.assertEqual(pj1, pj2)
E       AssertionError: <ecdsa.ellipticcurve.PointJacobi object at 0x7f4a0260f0d0> != <ecdsa.ellipticcurve.PointJacobi object at 0x7f4a0260f250>

Any idea what I'm doing wrong?

@adhintz
Copy link
Contributor Author

adhintz commented Jul 22, 2020

I figured out my question on the equality comparison of PointJacobi. CurveFp has __eq__ defined, but not __ne__. I'll add this to class CurveFp:

    def __ne__(self, other):
        return not self.__eq__(other)

…ing to acquire the lock, it now returns None. Think that's fine or prefer something else?

Added `__ne__` for CurveFp.

I think `test_inaquality_points_diff_types` from test_ellipticcurve.py was not actually testing equality. Because there was no `__ne__`, it was falling back to identity comparison, causing the test to pass. Now that there's a `__ne__`, it was getting NotImplemented which surprisingly bool casts to True. I changed the `__eq__` to return False for different types.

Fix typo in `test_inaquality_points_diff_types` name.

As requested, ran black on modified files. It reformmated some lines I hadn't modified.
@adhintz adhintz requested a review from tomato42 July 22, 2020 20:34
Copy link
Member

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

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

It reformmated some lines I hadn't modified

likely new version, that's ok

For __ne__ use == instead of __eq__ so it handles NotImplemented when the types are different.
Remove the not needed state declaration.
Copy link
Member

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@tomato42 tomato42 merged commit fc11345 into tlsfuzzer:master Jul 24, 2020
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