[ https://issues.apache.org/jira/browse/ZOOKEEPER-2890?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16153522#comment-16153522 ]
Alexander A. Strelets commented on ZOOKEEPER-2890: -------------------------------------------------- Also I have briefly verified the remaining cases in the _deserialize_response()_. As soon as all of them except _case COMPLETION_VOID_ and the mentioned _case COMPLETION_STRING_ are not used for _multi_ response, there is no problem with them. However, if some of them are included into _multi_ response in further releases, they also must be patched in the same way. Here is the brief: case COMPLETION_DATA: // get response deserialize_Buffer(in, "data", &v->data) may left v->data uninitialized ... then deallocate_Buffer(&v->data) will free by uninitialized v->data case COMPLETION_STRINGLIST: // get_children response deserialize_String_vector(in, "children", &v->children) calls start_vector(in, tag, &v->count) which may left v->children.count uninitialized ... then deserialize_String_vector() calls calloc(v->count, sizeof(*v->data)) which may face insufficient memory if v->count is a big number ... or at least lead to significant delay in executing the further for(i=0;i<v->count;i++) case COMPLETION_STRINGLIST_STAT: // get_children2 response the same as the above case case COMPLETION_STRING: // create response described in the main part of the ticket deserialize_String(in, "path", &v->path) may left v->path uninitialized ... then deallocate_String(&v->path) will free by uninitialized v->data case COMPLETION_ACLLIST: // get_acl response deserialize_ACL_vector(in, "acl", &v->acl) calls start_vector(in, tag, &v->count) which may left v->acl.count uninitialized ... then deserialize_ACL_vector() calls calloc(v->count, sizeof(*v->data)) which may face insufficient memory if v->count is a big number ... or at least lead to significant delay in executing the further for(i=0;i<v->count;i++) On the other hand: case COMPLETION_STAT -- looks safe as soon as _free()_ is not called for the _struct Stat_. > Local automatic variable is left uninitialized and then freed. > -------------------------------------------------------------- > > Key: ZOOKEEPER-2890 > URL: https://issues.apache.org/jira/browse/ZOOKEEPER-2890 > 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_response()_*, in _case COMPLETION_STRING_, uses local > automatic variable *_struct CreateResponse res_* which is +left > uninitialized+ and passed to the function _deserialize_GetACLResponse()_ and > then to _deallocate_GetACLResponse()_. > The _deserialize_ function, which is called the first, is expected to assign > the _res_ variable with a value from the parsed _struct iarchive *ia_. But, > if _ia_ contains for example insufficient amount of bytes the > _deserialize_String()_ function refuses of assigning a value to _res_, and > _res_ stays uninitialized (the true case is described below). Then, the > _deallocate_ function calls _deallocate_String()_ passing uninitialized _res_ > with arguments. If incidentally the memory region in the program stack under > the _res_ was not equal to NULL, the last call +leads to _free()_ by invalid > address+. > The true case: this happens when an active _multi_ request with _create_ > sub-request is completed on call to _zookeeper_close()_ with the so called > "Fake response" which is fabricated by the function _free_completions()_. > Such response includes only the header but +zero bytes for the body+. The > significant condition is that the _create_ request is not a stand-alone one, > but namely a sub-request within the _multi_ request. In this case the > _deserialize_response()_ is called recursively (for each sub-request), and > when it is called for the _create_ subrequest (from the nested > _deserialize_multi()_) the _failed_ parameter is assigned with false (0), so > the _if (failed)_ condition branches to the _else_ part. Note that in the > stand-alone create-request case this does not occur. > *I suspect this may happen not only due to call to _zookeeper_close()_ but on > reception of a true multi-response from the server* containing insufficient > number of bytes (I'm not sure if it can be a proper response from the server > with an error overall status and empty or insufficient payload). -- This message was sent by Atlassian JIRA (v6.4.14#64029)