[jira] [Commented] (ZOOKEEPER-2891) SIGABRT from assert during fake completion on zookeeper_close.

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2891?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16199393#comment-16199393
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2891:
---

Github user phunt commented on the issue:

https://github.com/apache/zookeeper/pull/360
  
ps. You accidentally included the patch for 2890 in this PR, perhaps you 
can fix that at the same time? Thx.


> SIGABRT from assert during fake completion on zookeeper_close.
> --
>
> Key: ZOOKEEPER-2891
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2891
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: c client
>Affects Versions: 3.4.10
> Environment: Linux ubuntu 4.4.0-87-generic
> gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609
> https://github.com/apache/zookeeper.git
> branch-3.4
>Reporter: Alexander A. Strelets
>Assignee: Alexander A. Strelets
>Priority: Critical
>  Labels: easyfix
> Fix For: 3.4.10
>
>
> ZooKeeper C Client *+single thread+* build
> Function *_deserialize_multi()_* hits *_assert(entry)_* when called for the 
> so called "Fake response" which is fabricated by the function 
> _free_completions()_ for example when _zookeeper_close()_ is called while 
> there is a pending _multi_ request.
> Such fake response includes only the header but zero bytes for the body. Due 
> to this {{deserialize_MultiHeader(ia, "multiheader", )}}, which is 
> called repeatedly for each {{completion_list_t *entry = 
> dequeue_completion(clist)}}, does not assign the _mhdr_ and keeps _mhdr.done 
> == 0_ as it was originally initialized. Consequently the _while (!mhdr.done)_ 
> does not ever end, and finally falls into the _assert(entry)_ with _entry == 
> NULL_ when all sub-requests are "completed". ~// Normally on my platform 
> assert raises SIGABRT.~
> I propose to instruct the _deserialize_multi()_ function to break the loop on 
> _entry == NULL_ if it was called for an unsuccessfull overal status of the 
> multi response, and in particular for the fake response having _ZCLOSING_ 
> (-116) status. I have introduced the _rc0_ parameter for this.
> *Another issue* with this function is that even if the while-loop exited 
> properly, this function returns _rc == 0_, and this return code +overrides+ 
> the true status value with {{rc = deserialize_multi(xid, cptr, ia, rc)}} in 
> the _deserialize_response()_ function. So, the _multi_ response callback 
> +handler would be called with _rc == ZOK_ instead of _rc == ZCLOSING_+ which 
> is strictly wrong.
> To fix this I propose initializing _rc_ with the introduced _rc0_ instead of 
> zero (which is _ZOK_ indeed).
> This is a proposed fix: https://github.com/apache/zookeeper/pull/360
> [upd]
> It looks like about the same problem is described in ZOOKEEPER-1636
> However, the patch proposed in this ticket also remedies the second linked 
> problem: reporting ZCLOSING status (as required) to the multi-request 
> completion handler.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2891) SIGABRT from assert during fake completion on zookeeper_close.

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2891?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16199391#comment-16199391
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2891:
---

Github user phunt commented on the issue:

https://github.com/apache/zookeeper/pull/360
  
@xoiss I like this patch but it's missing tests. Can you include the tests 
from 
https://issues.apache.org/jira/secure/attachment/12671215/ZOOKEEPER-1636.patch 
and resubmit?


> SIGABRT from assert during fake completion on zookeeper_close.
> --
>
> Key: ZOOKEEPER-2891
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2891
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: c client
>Affects Versions: 3.4.10
> Environment: Linux ubuntu 4.4.0-87-generic
> gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609
> https://github.com/apache/zookeeper.git
> branch-3.4
>Reporter: Alexander A. Strelets
>Assignee: Alexander A. Strelets
>Priority: Critical
>  Labels: easyfix
> Fix For: 3.4.10
>
>
> ZooKeeper C Client *+single thread+* build
> Function *_deserialize_multi()_* hits *_assert(entry)_* when called for the 
> so called "Fake response" which is fabricated by the function 
> _free_completions()_ for example when _zookeeper_close()_ is called while 
> there is a pending _multi_ request.
> Such fake response includes only the header but zero bytes for the body. Due 
> to this {{deserialize_MultiHeader(ia, "multiheader", )}}, which is 
> called repeatedly for each {{completion_list_t *entry = 
> dequeue_completion(clist)}}, does not assign the _mhdr_ and keeps _mhdr.done 
> == 0_ as it was originally initialized. Consequently the _while (!mhdr.done)_ 
> does not ever end, and finally falls into the _assert(entry)_ with _entry == 
> NULL_ when all sub-requests are "completed". ~// Normally on my platform 
> assert raises SIGABRT.~
> I propose to instruct the _deserialize_multi()_ function to break the loop on 
> _entry == NULL_ if it was called for an unsuccessfull overal status of the 
> multi response, and in particular for the fake response having _ZCLOSING_ 
> (-116) status. I have introduced the _rc0_ parameter for this.
> *Another issue* with this function is that even if the while-loop exited 
> properly, this function returns _rc == 0_, and this return code +overrides+ 
> the true status value with {{rc = deserialize_multi(xid, cptr, ia, rc)}} in 
> the _deserialize_response()_ function. So, the _multi_ response callback 
> +handler would be called with _rc == ZOK_ instead of _rc == ZCLOSING_+ which 
> is strictly wrong.
> To fix this I propose initializing _rc_ with the introduced _rc0_ instead of 
> zero (which is _ZOK_ indeed).
> This is a proposed fix: https://github.com/apache/zookeeper/pull/360
> [upd]
> It looks like about the same problem is described in ZOOKEEPER-1636
> However, the patch proposed in this ticket also remedies the second linked 
> problem: reporting ZCLOSING status (as required) to the multi-request 
> completion handler.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2891) SIGABRT from assert during fake completion on zookeeper_close.

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2891?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16184236#comment-16184236
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2891:
---

Github user phunt commented on the issue:

https://github.com/apache/zookeeper/pull/360
  
Hi @xoiss please update this PR to follow the guidelines in "generate a 
pull request" here: 
https://cwiki.apache.org/confluence/display/ZOOKEEPER/HowToContribute - in 
particular wrt the summary and description text which really helps folks 
identifying which PRs to prioritize. Thanks!


> SIGABRT from assert during fake completion on zookeeper_close.
> --
>
> Key: ZOOKEEPER-2891
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2891
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: c client
>Affects Versions: 3.4.10
> Environment: Linux ubuntu 4.4.0-87-generic
> gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609
> https://github.com/apache/zookeeper.git
> branch-3.4
>Reporter: Alexander A. Strelets
>Priority: Critical
>  Labels: easyfix
> Fix For: 3.4.10
>
>
> ZooKeeper C Client *+single thread+* build
> Function *_deserialize_multi()_* hits *_assert(entry)_* when called for the 
> so called "Fake response" which is fabricated by the function 
> _free_completions()_ for example when _zookeeper_close()_ is called while 
> there is a pending _multi_ request.
> Such fake response includes only the header but zero bytes for the body. Due 
> to this {{deserialize_MultiHeader(ia, "multiheader", )}}, which is 
> called repeatedly for each {{completion_list_t *entry = 
> dequeue_completion(clist)}}, does not assign the _mhdr_ and keeps _mhdr.done 
> == 0_ as it was originally initialized. Consequently the _while (!mhdr.done)_ 
> does not ever end, and finally falls into the _assert(entry)_ with _entry == 
> NULL_ when all sub-requests are "completed". ~// Normally on my platform 
> assert raises SIGABRT.~
> I propose to instruct the _deserialize_multi()_ function to break the loop on 
> _entry == NULL_ if it was called for an unsuccessfull overal status of the 
> multi response, and in particular for the fake response having _ZCLOSING_ 
> (-116) status. I have introduced the _rc0_ parameter for this.
> *Another issue* with this function is that even if the while-loop exited 
> properly, this function returns _rc == 0_, and this return code +overrides+ 
> the true status value with {{rc = deserialize_multi(xid, cptr, ia, rc)}} in 
> the _deserialize_response()_ function. So, the _multi_ response callback 
> +handler would be called with _rc == ZOK_ instead of _rc == ZCLOSING_+ which 
> is strictly wrong.
> To fix this I propose initializing _rc_ with the introduced _rc0_ instead of 
> zero (which is _ZOK_ indeed).
> This is a proposed fix: https://github.com/apache/zookeeper/pull/360
> [upd]
> It looks like about the same problem is described in ZOOKEEPER-1636
> However, the patch proposed in this ticket also remedies the second linked 
> problem: reporting ZCLOSING status (as required) to the multi-request 
> completion handler.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2891) SIGABRT from assert during fake completion on zookeeper_close.

[Hadoop QA (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2891?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16153637#comment-16153637
 ] 

Hadoop QA commented on ZOOKEEPER-2891:
--

+1 overall.  GitHub Pull Request  Build
  

+1 @author.  The patch does not contain any @author tags.

+0 tests included.  The patch appears to be a documentation patch that 
doesn't require tests.

+1 javadoc.  The javadoc tool did not generate any warning messages.

+1 javac.  The applied patch does not increase the total number of javac 
compiler warnings.

+1 findbugs.  The patch does not introduce any new Findbugs (version 3.0.1) 
warnings.

+1 release audit.  The applied patch does not increase the total number of 
release audit warnings.

+1 core tests.  The patch passed core unit tests.

+1 contrib tests.  The patch passed contrib unit tests.

Test results: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/996//testReport/
Findbugs warnings: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/996//artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output: 
https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/996//console

This message is automatically generated.

> SIGABRT from assert during fake completion on zookeeper_close.
> --
>
> Key: ZOOKEEPER-2891
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2891
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: c client
>Affects Versions: 3.4.10
> Environment: Linux ubuntu 4.4.0-87-generic
> gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609
> https://github.com/apache/zookeeper.git
> branch-3.4
>Reporter: Alexander A. Strelets
>Priority: Critical
>  Labels: easyfix
> Fix For: 3.4.10
>
>
> Function *_deserialize_multi()_* hits *_assert(entry)_* when called for the 
> so called "Fake response" which is fabricated by the function 
> _free_completions()_ for example when _zookeeper_close()_ is called while 
> there is a pending _multi_ request.
> Such fake response includes only the header but zero bytes for the body. Due 
> to this {{deserialize_MultiHeader(ia, "multiheader", )}}, which is 
> called repeatedly for each {{completion_list_t *entry = 
> dequeue_completion(clist)}}, does not assign the _mhdr_ and keeps _mhdr.done 
> == 0_ as it was originally initialized. Consequently the _while (!mhdr.done)_ 
> does not ever end, and finally falls into the _assert(entry)_ with _entry == 
> NULL_ when all sub-requests are "completed". ~// Normally on my platform 
> assert raises SIGABRT.~
> I propose to instruct the _deserialize_multi()_ function to break the loop on 
> _entry == NULL_ if it was called for an unsuccessfull overal status of the 
> multi response, and in particular for the fake response having _ZCLOSING_ 
> (-116) status. I have introduced the _rc0_ parameter for this.
> *Another issue* with this function is that even if the while-loop exited 
> properly, this function returns _rc == 0_, and this return code +overrides+ 
> the true status value with {{rc = deserialize_multi(xid, cptr, ia, rc)}} in 
> the _deserialize_response()_ function. So, the _multi_ response callback 
> +handler would be called with _rc == ZOK_ instead of _rc == ZCLOSING_+ which 
> is strictly wrong.
> To fix this I propose initializing _rc_ with the introduced _rc0_ instead of 
> zero (which is _ZOK_ indeed).
> This is a proposed fix: https://github.com/apache/zookeeper/pull/360



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

[jira] [Commented] (ZOOKEEPER-2891) SIGABRT from assert during fake completion on zookeeper_close.

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-2891?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16153585#comment-16153585
 ] 

ASF GitHub Bot commented on ZOOKEEPER-2891:
---

GitHub user xoiss opened a pull request:

https://github.com/apache/zookeeper/pull/360

branch-3.4 -- bugfix -- ZOOKEEPER-2891

Fixes https://issues.apache.org/jira/browse/ZOOKEEPER-2891

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/xoiss/zookeeper 
branch-3.4-bugfix-zookeeper-2891

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/zookeeper/pull/360.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #360


commit 2dc3664e52a0f1ae82c5c4fdc800921548bfc087
Author: xoiss 
Date:   2017-09-05T11:28:36Z

ZOOKEEPER-2890 - fix freing by uninitialized address.

commit b6da551a38bcfe834038c44f94da0bbfb2c881a5
Author: xoiss 
Date:   2017-09-05T12:48:33Z

ZOOKEEPER-2891 - fix endless loop and assertion on fake multi response.




> SIGABRT from assert during fake completion on zookeeper_close.
> --
>
> Key: ZOOKEEPER-2891
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2891
> Project: ZooKeeper
>  Issue Type: Bug
>  Components: c client
>Affects Versions: 3.4.10
> Environment: Linux ubuntu 4.4.0-87-generic
> gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609
> https://github.com/apache/zookeeper.git
> branch-3.4
>Reporter: Alexander A. Strelets
>Priority: Critical
>  Labels: easyfix
> Fix For: 3.4.10
>
>
> Function *_deserialize_multi()_* hits *_assert(entry)_* when called for the 
> so called "Fake response" which is fabricated by the function 
> _free_completions()_ for example when _zookeeper_close()_ is called while 
> there is a pending _multi_ request.
> Such fake response includes only the header but zero bytes for the body. Due 
> to this {{deserialize_MultiHeader(ia, "multiheader", )}}, which is 
> called repeatedly for each {{completion_list_t *entry = 
> dequeue_completion(clist)}}, does not assign the _mhdr_ and keeps _mhdr.done 
> == 0_ as it was originally initialized. Consequently the _while (!mhdr.done)_ 
> does not ever end, and finally falls into the _assert(entry)_ with _entry == 
> NULL_ when all sub-requests are "completed". ~// Normally on my platform 
> assert raises SIGABRT.~
> I propose to instruct the _deserialize_multi()_ function to break the loop on 
> _entry == NULL_ if it was called for an unsuccessfull overal status of the 
> multi response, and in particular for the fake response having _ZCLOSING_ 
> (-116) status. I have introduced the _rc0_ parameter for this.
> *Another issue* with this function is that even if the while-loop exited 
> properly, this function returns _rc == 0_, and this return code +overrides+ 
> the true status value with {{rc = deserialize_multi(xid, cptr, ia, rc)}} in 
> the _deserialize_response()_ function. So, the _multi_ response callback 
> +handler would be called with _rc == ZOK_ instead of _rc == ZCLOSING_+ which 
> is strictly wrong.
> To fix this I propose initializing _rc_ with the introduced _rc0_ instead of 
> zero (which is _ZOK_ indeed).



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)