ZOOKEEPER-2891: Invalid processing of zookeeper_close for mutli-request #999

xoiss · 2019-06-23T18:17:05Z

When I call zookeeper_close() while there is a pending multi request, I expect the request completes with ZCLOSING status.

But with the existing code I actually get the following:

the program exits with SIGABRT from assert(entry) in deserialize_multi()
and even if I remove this assertion and just break the enclosing loop, the returned status is ZOK but not ZCLOSING

So, there are two defects with processing calls to zookeeper_close() for pending multi requests: improper assertion in implementation and invalid status in confirmation.

Proposed changes remove these defects.

For more details see: https://issues.apache.org/jira/browse/ZOOKEEPER-2891

xoiss · 2019-06-23T19:09:05Z

Hello!

This is a reworked/updated/improved PR.

The previous one: #360

eolivelli

Thos change makes sense to me.

I do not have so much experience with c client, so I can't say we are covering all of the aspects of this problem.

As said in another PR we should add a test to demonstrate the problem and verify that we have fixed it.

I will let other reviewers tell their opinion

Thanks

zookeeper-client/zookeeper-client-c/src/zookeeper.c

xoiss · 2019-07-02T14:16:43Z

add a test to demonstrate the problem and verify that we have fixed it

Done!

I have provided two tests:

testCloseWhileMultiInProgressFromMain
testCloseWhileMultiInProgressFromCompletion

The reason to have namely two tests is about the same as for ZOOKEEPER-2894 (see #1000). So, it's just two subcases of the same problem.

Note: Right now, the first test of two is temporarily disabled because it requires fixes from ZOOKEEPER-2894. When it's merged, I'll rebase this PR and enable this test.

Ok, BEFORE this patch both tests fail with src/zookeeper.c:2033: deserialize_multi: Assertion 'entry' failed (for the first test, assuming ZOOKEEPER-2894 is already merged, otherwise it fails with completion lost).

And after the patch both tests pass successfully (again, for the first test, assuming ZOOKEEPER-2894 is already merged).

Can you explain why this is the right point to exit the loop and the method?

What is the alternative??
Please, read this: https://issues.apache.org/jira/browse/ZOOKEEPER-2891
I have revised it again now, and indeed, I don't have more detailed explanation.
The best is try to reproduce this under debugger. You may use unit-tests published here.
If you still have questions, please, formulate more specific question, or a use-case (scenario) that I can reproduce.

To my eyes, it is cleaner to have rc_hint == ZOK as a conditional on the while statement or perhaps a return rc down a few lines where it is set on error.

If we put rc_hint == ZOK directly into the while-conditional-expression then we have to put there also (1) assignment of entry and (2) analyzing the value assigned to entry -- i.e., two code lines: completion_list_t *entry = dequeue_completion(clist); and if (entry == NULL). Note also that entry is a local variable of the while-loop-body, and C99 does not allow local variable declarations right in the while-expression.
So, due to this, it's better to keep things at there original places. I understand that having additional loop-termination point is not good enough, but sometimes it's a kinda tradeoff. I wish to keep things as close to the code-before-patch as possible, to minimize the diff.

Could we also return immediately from the method is rc_hint is not 0?

Currently 'yes', because there is nothing between the while-loop-body-end and the final return statement. But there is no lifetime warranty that nothing would be put here between them.
Actually, I prefer break here because it performs the minimum of the necessary work -- I'm talking about the "Necessity and Sufficiency" -- here return is sufficient, but much more than necessary.

xoiss · 2019-07-02T16:22:08Z

rebased on 4a5d596
ZOOKEEPER-3389: Zookeeper does not export all required packages in OSGi (for Curator)

xoiss · 2019-07-11T08:22:21Z

Rebased on the result of merging ZOOKEEPER-2894
Enabled testCloseWhileMultiInProgressFromMain that required ZOOKEEPER-2894

xoiss · 2019-07-11T09:02:40Z

@phunt @anmolnar @eolivelli @hanm

Hello!

It looks, I'm ready with this patch too.
So, you may revise it.

But I have three questions:

How can I see the results of ZooKeeper C Client unit-tests run? Are the unit-tests run indeed? I suspect, No, because I don't see them in the log: https://travis-ci.org/apache/zookeeper/jobs/557205041#L1
What happened with Jenkins? It's not listed in the precommit checks.
Last time UTs run by Jenkins failed to pass testCloseWhileMultiInProgressFromCompletion with strange error probably related to function-stack-frame overflow. I didn't succeed in reproducing it on my desktop. And now I just can't see if the problem still exists. This is the last report I'm talking:

     [exec]      [exec] Zookeeper_operations::testCloseWhileMultiInProgressFromCompletionMakefile:1909: recipe for target 'run-check' failed
     [exec]      [exec] *** stack smashing detected ***: ./zktest-st terminated

https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/4058/console

Thank you!

xoiss · 2019-07-29T13:19:55Z

Rebased on
commit c056703
Author: Patrick Hunt phunt@apache.org 2019-07-12 08:32:42
Committer: Enrico Olivelli eolivelli@apache.org 2019-07-13 09:45:14
ZOOKEEPER-3441: OWASP is flagging jackson-databind-2.9.9.jar for CVE-…

xoiss · 2019-07-29T17:13:17Z

Resolved "stack smashing detected" in Zookeeper_operations::testCloseWhileMultiInProgressFromCompletionMakefile

// the reason was locally allocated zoo_op_result_t

Now it works fine.

What happened with junit tests?
Some of them failed, and due to this cppunit-test do not start in Jenkins.
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/4083/consoleFull

@phunt @anmolnar @eolivelli @hanm

xoiss · 2019-07-30T15:44:32Z

... hmmm, maybe it's better to wait for #1030

This is a backport of #717 to `branch-3.4` For some reason it was NOT merged to `branch-3.4`, but only to `branch-3.5` and `master`: - 4bd32be - b1fd480 Here, it's almost a cherry-pick of the original patch, but slightly adopted to specific of `branch-3.4`. Namely, `cleanup_failed_multi()` and `deserialize_multi()` do not have the first parameter `zhandle_t *zh`. ---- I wish to backport this also to use it as the base for my #999 Author: Alexander A. Strelets <streletsaa@gmail.com> Reviewers: Andor Molnár <andor@apache.org>, Michael Han <hanm@apache.org> Closes #1030 from xoiss/ZOOKEEPER-1636

anmolnar · 2019-08-02T12:16:45Z

@xoiss #1030 merged. Please rebase this one.

xoiss · 2019-08-02T16:35:01Z

@xoiss #1030 merged. Please rebase this one.

rebased

Hello!

As it turned out, #1030, #717 (ZOOKEEPER-1636) is 100% enough to close ZOOKEEPER-2891 also.

And as soon as #1030, #717 cover the more common case (general "unsuccessful status", but not only ZCLOSING), and #717 is already in the master -- surely, I would prefer to withdraw my solution in this PR in flavour of that one in #1030 #717.

// Many thanks to @mkedwards for his idea given in #717 !

Hence, there are two unit-tests proposed in this PR. If you wish, we can merge them into branch-3.4, branch-3.5 and master. If it's necessary, I can open a dedicated PR for them. What do you think?

// I've edited this PR. Namely, I've removed patches to the Client code, but left only two unit-tests. So, this PR now contains only unit-tests.

xoiss · 2019-08-05T13:44:35Z

^ up

@phunt @anmolnar @eolivelli @hanm

When I call `zookeeper_close()` while there is a pending multi request, I expect the request completes with `ZCLOSING` status. But with the existing code I actually get the following: * the program exits with `SIGABRT` from `assert(entry)` in `deserialize_multi()` * and even if I remove this assertion and just break the enclosing loop, the returned status is `ZOK` but not `ZCLOSING` So, there are two defects with processing calls to `zookeeper_close()` for pending multi requests: improper assertion in implementation and invalid status in confirmation. Proposed changes remove these defects. For more details see: https://issues.apache.org/jira/browse/ZOOKEEPER-2891 Author: Alexander A. Strelets <streletsaa@gmail.com> Reviewers: Michael Han <hanm@apache.org> Closes #999 from xoiss/ZOOKEEPER-2891

hanm · 2019-08-06T00:02:06Z

merged to 3.4. Please close the pr @xoiss

anmolnar · 2019-08-06T15:10:12Z

I can close it. Thanks @hanm @xoiss !

xoiss · 2019-08-06T15:22:49Z

Thank you!

But, why not to cherry pick these to branch-3.5 and master also?

@anmolnar @hanm

anmolnar · 2019-08-06T15:54:44Z

@xoiss I haven't noticed that this one targets 3.4, sorry. The usual way of contributions is to create patch for master first that we can cherry-pick to other branches, not the other way around. Let me take care of this.

When I call `zookeeper_close()` while there is a pending multi request, I expect the request completes with `ZCLOSING` status. But with the existing code I actually get the following: * the program exits with `SIGABRT` from `assert(entry)` in `deserialize_multi()` * and even if I remove this assertion and just break the enclosing loop, the returned status is `ZOK` but not `ZCLOSING` So, there are two defects with processing calls to `zookeeper_close()` for pending multi requests: improper assertion in implementation and invalid status in confirmation. Proposed changes remove these defects. For more details see: https://issues.apache.org/jira/browse/ZOOKEEPER-2891 Author: Alexander A. Strelets <streletsaa@gmail.com> Reviewers: Michael Han <hanm@apache.org> Closes #999 from xoiss/ZOOKEEPER-2891

anmolnar · 2019-08-06T16:03:19Z

Cherry-picked to master and 3.5 branches.

When I call `zookeeper_close()` while there is a pending multi request, I expect the request completes with `ZCLOSING` status. But with the existing code I actually get the following: * the program exits with `SIGABRT` from `assert(entry)` in `deserialize_multi()` * and even if I remove this assertion and just break the enclosing loop, the returned status is `ZOK` but not `ZCLOSING` So, there are two defects with processing calls to `zookeeper_close()` for pending multi requests: improper assertion in implementation and invalid status in confirmation. Proposed changes remove these defects. For more details see: https://issues.apache.org/jira/browse/ZOOKEEPER-2891 Author: Alexander A. Strelets <streletsaa@gmail.com> Reviewers: Michael Han <hanm@apache.org> Closes apache#999 from xoiss/ZOOKEEPER-2891

xoiss · 2019-08-08T09:31:09Z

... is to create patch for master first ...

Yes, I've got it.

Ok, with this commit, it's the whole amount of patches I had.
Thank you a lot friends!!

anmolnar · 2019-08-08T14:56:32Z

@xoiss Thanks for the contribution.

When I call `zookeeper_close()` while there is a pending multi request, I expect the request completes with `ZCLOSING` status. But with the existing code I actually get the following: * the program exits with `SIGABRT` from `assert(entry)` in `deserialize_multi()` * and even if I remove this assertion and just break the enclosing loop, the returned status is `ZOK` but not `ZCLOSING` So, there are two defects with processing calls to `zookeeper_close()` for pending multi requests: improper assertion in implementation and invalid status in confirmation. Proposed changes remove these defects. For more details see: https://issues.apache.org/jira/browse/ZOOKEEPER-2891 Author: Alexander A. Strelets <streletsaa@gmail.com> Reviewers: Michael Han <hanm@apache.org> Closes apache#999 from xoiss/ZOOKEEPER-2891

xoiss mentioned this pull request Jun 23, 2019

branch-3.4 -- bugfix -- ZOOKEEPER-2891 #360

Closed

xoiss mentioned this pull request Jun 23, 2019

ZOOKEEPER-2894: Memory and completions leak on zookeeper_close #1000

Closed

eolivelli reviewed Jun 24, 2019

View reviewed changes

enixon reviewed Jun 24, 2019

View reviewed changes

zookeeper-client/zookeeper-client-c/src/zookeeper.c Outdated Show resolved Hide resolved

xoiss force-pushed the ZOOKEEPER-2891 branch from dd81c39 to 2053f56 Compare July 2, 2019 13:38

xoiss force-pushed the ZOOKEEPER-2891 branch from 2053f56 to a266fa8 Compare July 2, 2019 16:21

xoiss force-pushed the ZOOKEEPER-2891 branch 2 times, most recently from 1711b07 to e2b1cd7 Compare July 11, 2019 08:20

xoiss force-pushed the ZOOKEEPER-2891 branch 2 times, most recently from bc321e7 to 707aa4e Compare July 29, 2019 13:12

xoiss force-pushed the ZOOKEEPER-2891 branch 2 times, most recently from 14185de to 1e47602 Compare July 29, 2019 14:41

xoiss mentioned this pull request Jul 30, 2019

ZOOKEEPER-1636: cleanup completion list of a failed multi request #1030

Closed

xoiss force-pushed the ZOOKEEPER-2891 branch from 1e47602 to 46f8f0a Compare July 30, 2019 21:18

ZOOKEEPER-2891: Invalid processing of zookeeper_close for mutli-request

2e1c523

xoiss force-pushed the ZOOKEEPER-2891 branch from 46f8f0a to 2e1c523 Compare August 2, 2019 16:21

hanm approved these changes Aug 5, 2019

View reviewed changes

anmolnar closed this Aug 6, 2019

ZOOKEEPER-2891: Invalid processing of zookeeper_close for mutli-request #999

ZOOKEEPER-2891: Invalid processing of zookeeper_close for mutli-request #999

Uh oh!

Conversation

xoiss commented Jun 23, 2019

Uh oh!

xoiss commented Jun 23, 2019

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xoiss commented Jul 2, 2019

Uh oh!

xoiss commented Jul 2, 2019

Uh oh!

xoiss commented Jul 11, 2019

Uh oh!

xoiss commented Jul 11, 2019

Uh oh!

xoiss commented Jul 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xoiss commented Jul 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xoiss commented Jul 30, 2019

Uh oh!

anmolnar commented Aug 2, 2019

Uh oh!

xoiss commented Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xoiss commented Aug 5, 2019

Uh oh!

hanm commented Aug 6, 2019

Uh oh!

anmolnar commented Aug 6, 2019

Uh oh!

xoiss commented Aug 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anmolnar commented Aug 6, 2019

Uh oh!

anmolnar commented Aug 6, 2019

Uh oh!

xoiss commented Aug 8, 2019

Uh oh!

anmolnar commented Aug 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

xoiss commented Jul 29, 2019 •

edited

Loading

xoiss commented Jul 29, 2019 •

edited

Loading

xoiss commented Aug 2, 2019 •

edited

Loading

xoiss commented Aug 6, 2019 •

edited

Loading