zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexander A. Strelets (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (ZOOKEEPER-2890) Local automatic variable is left uninitialized and then freed.
Date Tue, 05 Sep 2017 11:51:00 GMT

    [ 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)

Mime
View raw message