Details
-
Type:
Bug
-
Status: Closed
-
Priority:
Critical
-
Resolution: Fixed
-
Affects Version/s: 3.3.1
-
Component/s: c client, contrib-bindings
-
Labels:None
-
Hadoop Flags:Reviewed
Description
the c-client / zkpython wrapper invokes already freed watcher callback
steps to reproduce:
0. start a zookeper server on your machine
1. run the attached python script
2. suspend the zookeeper server process (e.g. using `pkill -STOP -f org.apache.zookeeper.server.quorum.QuorumPeerMain` )
3. wait until the connection and the node observer fired with a session event
4. resume the zookeeper server process (e.g. using `pkill -CONT -f org.apache.zookeeper.server.quorum.QuorumPeerMain` )
-> the client tries to dispatch the node observer function again, but it was already freed -> double free corruption
Issue Links
- duplicates
-
ZOOKEEPER-740
zkpython leading to segfault on zookeeper
-
- Resolved
-
- is duplicated by
-
ZOOKEEPER-890
C client invokes watcher callbacks multiple times
-
- Resolved
-
Activity
I just committed this to origin/branch-3.3 and origin/trunk.
Thanks both!
Patch based on the 3.3 branch attached (ZOOKEEPER-888-3.3.patch). Verified that unit tests pass with the changes, including the new watcher_test.
Can we backport the 853 change into 3.3, but not change the API? Big overhead or straightforward/simple?
The patch as it stands relies on ZOOKEEPER-853 (which it fixes) which is not in 3.3 as it is a small API change - it changes is_unrecoverable to return Python True or False, rather than ZINVALIDSTATE.
So I'm not certain about what to do here - we try not to change APIs between minor versions. However, this is a very minor change, and this patch fixes a significant bug. I'm inclined to commit both 853 and this patch to 3.3 as well as trunk, and put a note in the release notes.
Any objections?
Updated ZOOKEEPER-888.patch with the following changes:
- Fixed zookeeper.is_unrecoverable to return the correct value, it was returning false in all cases.
- Added watcher_test.py to cover the issue this patch fixes. Verified that it crashes before patching and succeeds afterward.
The patch looks good to me - thanks!
Could you add a test case that verifies the correct behaviour, if possible? (I appreciate it can be hard to fake unrecoverable session errors). We keep circling around the correct behaviour for this code block, and I'd like to capture it in a test suite.
Improved patch attached. Before, watcher_dispatch would unconditionally free non-global watcher objects.
Any number of recoverable session state change events may be sent to the watcher. This change frees the watcher only on the last callback- a data change event or unrecoverable session state change.
Path that prevents freeing a watcher in response to a session event, per the feedback in ZOOKEEPER-890.
Borderline blocker, Henry any insight on this? Something that can be addressed for 3.3.2?
Integrated in ZooKeeper-trunk #972 (See https://hudson.apache.org/hudson/job/ZooKeeper-trunk/972/)
ZOOKEEPER-888: c-client / zkpython: Double free corruption on nodewatcher (Austin Shoemaker via henryr)