Skip to content

Loading…

Fixed list/hash autofree strategy #103

Merged
merged 2 commits into from

2 participants

@hintjens
The ZeroMQ project member

Was breaking systematically since it depended on caller to strdup passed values; omitting this created a hard-to-find error. Now the insert/update methods all duplicate the passed value/item for dynamic containers. If callers also duplicate the item the result is a dangling allocation, which is easy to find.

hintjens added some commits
@hintjens hintjens Replaced TRUE/FALSE with true/false 8f8528a
@hintjens hintjens Improved containers handling of autofree items
* zhash duplicates item value if container has autofree set
* zlist duplicates item if container has autofree set

This is to get around anti-pattern where caller is responsible for strdup,
and forgetting it causes a double-free. It's easier to detect a dangling
allocation than a double-free.
8f2904f
@felipecruz felipecruz merged commit 799a880 into zeromq:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 8, 2012
  1. @hintjens
  2. @hintjens

    Improved containers handling of autofree items

    hintjens committed
    * zhash duplicates item value if container has autofree set
    * zlist duplicates item if container has autofree set
    
    This is to get around anti-pattern where caller is responsible for strdup,
    and forgetting it causes a double-free. It's easier to detect a dangling
    allocation than a double-free.
Showing with 45 additions and 29 deletions.
  1. +2 −2 src/czmq_selftest.c
  2. +2 −2 src/zctx.c
  3. +1 −1 src/zfile.c
  4. +7 −7 src/zframe.c
  5. +16 −8 src/zhash.c
  6. +8 −0 src/zlist.c
  7. +3 −3 src/zloop.c
  8. +1 −1 src/zmsg.c
  9. +3 −3 src/zsocket.c
  10. +2 −2 src/zstr.c
View
4 src/czmq_selftest.c
@@ -32,9 +32,9 @@ int main (int argc, char *argv [])
{
bool verbose;
if (argc == 2 && streq (argv [1], "-v"))
- verbose = TRUE;
+ verbose = true;
else
- verbose = FALSE;
+ verbose = false;
printf ("Running czmq self tests...\n");
View
4 src/zctx.c
@@ -61,7 +61,7 @@
struct _zctx_t {
void *context; // Our 0MQ context
zlist_t *sockets; // Sockets held by this thread
- bool main; // TRUE if we're the main thread
+ bool main; // True if we're the main thread
int iothreads; // Number of IO threads, default 1
int linger; // Linger timeout, default 0
};
@@ -99,7 +99,7 @@ zctx_new (void)
return NULL;
}
self->iothreads = 1;
- self->main = TRUE;
+ self->main = true;
#if defined (__UNIX__)
// Install signal handler for SIGINT and SIGTERM
View
2 src/zfile.c
@@ -131,7 +131,7 @@ zfile_test (bool verbose)
assert (rc == -1);
rc = zfile_exists ("nosuchfile");
- assert (rc == FALSE);
+ assert (rc != true);
rc = (int) zfile_size ("nosuchfile");
assert (rc == -1);
View
14 src/zframe.c
@@ -272,7 +272,7 @@ zframe_strdup (zframe_t *self)
// --------------------------------------------------------------------------
-// Return TRUE if frame body is equal to string, excluding terminator
+// Return true if frame body is equal to string, excluding terminator
bool
zframe_streq (zframe_t *self, const char *string)
@@ -280,9 +280,9 @@ zframe_streq (zframe_t *self, const char *string)
assert (self);
if (zframe_size (self) == strlen (string)
&& memcmp (zframe_data (self), string, strlen (string)) == 0)
- return TRUE;
+ return true;
else
- return FALSE;
+ return false;
}
@@ -308,20 +308,20 @@ zframe_zero_copy (zframe_t *self)
// --------------------------------------------------------------------------
-// Return TRUE if two frames have identical size and data
+// Return true if two frames have identical size and data
bool
zframe_eq (zframe_t *self, zframe_t *other)
{
if (!self || !other)
- return FALSE;
+ return false;
else
if (zframe_size (self) == zframe_size (other)
&& memcmp (zframe_data (self), zframe_data (other),
zframe_size (self)) == 0)
- return TRUE;
+ return true;
else
- return FALSE;
+ return false;
}
View
24 src/zhash.c
@@ -194,7 +194,7 @@ zhash_destroy (zhash_t **self_p)
item_t *cur_item = self->items [index];
while (cur_item) {
item_t *next_item = cur_item->next;
- s_item_destroy (self, cur_item, TRUE);
+ s_item_destroy (self, cur_item, true);
cur_item = next_item;
}
}
@@ -246,6 +246,10 @@ zhash_insert (zhash_t *self, const char *key, void *value)
self->items = new_items;
self->limit = new_limit;
}
+ // If necessary, take duplicate of item (string) value
+ if (self->autofree)
+ value = strdup ((char *) value);
+
return s_item_insert (self, key, value)? 0: -1;
}
@@ -268,6 +272,10 @@ zhash_update (zhash_t *self, const char *key, void *value)
else
if (self->autofree)
free (item->value);
+
+ // If necessary, take duplicate of item (string) value
+ if (self->autofree)
+ value = strdup ((char *) value);
item->value = value;
}
else
@@ -287,7 +295,7 @@ zhash_delete (zhash_t *self, const char *key)
item_t *item = s_item_lookup (self, key);
if (item)
- s_item_destroy (self, item, TRUE);
+ s_item_destroy (self, item, true);
}
@@ -317,7 +325,7 @@ zhash_rename (zhash_t *self, const char *old_key, const char *new_key)
{
item_t *item = s_item_lookup (self, old_key);
if (item) {
- s_item_destroy (self, item, FALSE);
+ s_item_destroy (self, item, false);
item_t *new_item = s_item_lookup (self, new_key);
if (new_item == NULL) {
free (item->key);
@@ -389,7 +397,7 @@ zhash_dup (zhash_t *self)
for (index = 0; index != self->limit; index++) {
item_t *item = self->items [index];
while (item) {
- zhash_insert (copy, item->key, strdup (item->value));
+ zhash_insert (copy, item->key, item->value);
item = item->next;
}
}
@@ -412,7 +420,7 @@ zhash_keys (zhash_t *self)
for (index = 0; index != self->limit; index++) {
item_t *item = self->items [index];
while (item) {
- zlist_append (keys, strdup (item->key));
+ zlist_append (keys, item->key);
item = item->next;
}
}
@@ -498,7 +506,7 @@ zhash_load (zhash_t *self, char *filename)
if (!equals)
break; // Some error, stop parsing it
*equals++ = 0;
- zhash_update (self, buffer, strdup (equals));
+ zhash_update (self, buffer, equals);
}
fclose (handle);
return 0;
@@ -617,12 +625,12 @@ zhash_test (int verbose)
item = zhash_lookup (hash, testset [testnbr].name);
assert (item);
zhash_delete (hash, testset [testnbr].name);
- testset [testnbr].exists = FALSE;
+ testset [testnbr].exists = false;
}
else {
sprintf (testset [testnbr].name, "%x-%x", rand (), rand ());
if (zhash_insert (hash, testset [testnbr].name, "") == 0)
- testset [testnbr].exists = TRUE;
+ testset [testnbr].exists = true;
}
}
// Test 10K lookups
View
8 src/zlist.c
@@ -177,6 +177,10 @@ zlist_append (zlist_t *self, void *item)
if (!node)
return -1;
+ // If necessary, take duplicate of (string) item
+ if (self->autofree)
+ item = strdup ((char *) item);
+
node->item = item;
if (self->tail)
self->tail->next = node;
@@ -202,6 +206,10 @@ zlist_push (zlist_t *self, void *item)
if (!node)
return -1;
+ // If necessary, take duplicate of (string) item
+ if (self->autofree)
+ item = strdup ((char *) item);
+
node->item = item;
node->next = self->head;
self->head = node;
View
6 src/zloop.c
@@ -127,7 +127,7 @@ s_rebuild_pollset (zloop_t *self)
item_nbr++;
poller = (s_poller_t *) zlist_next (self->pollers);
}
- self->dirty = FALSE;
+ self->dirty = false;
return 0;
}
@@ -234,7 +234,7 @@ zloop_poller (zloop_t *self, zmq_pollitem_t *item, zloop_fn handler, void *arg)
if (zlist_push (self->pollers, poller))
return -1;
- self->dirty = TRUE;
+ self->dirty = true;
if (self->verbose)
zclock_log ("I: zloop: register %s poller (%p, %d)",
item->socket? zsocket_type_str (item->socket): "FD",
@@ -263,7 +263,7 @@ zloop_poller_end (zloop_t *self, zmq_pollitem_t *item)
|| (item->fd && item->fd == poller->item.fd)) {
zlist_remove (self->pollers, poller);
free (poller);
- self->dirty = TRUE;
+ self->dirty = true;
}
poller = (s_poller_t *) zlist_next (self->pollers);
}
View
2 src/zmsg.c
@@ -457,7 +457,7 @@ zmsg_load (zmsg_t *self, FILE *file)
if (!self)
return NULL;
- while (TRUE) {
+ while (true) {
size_t frame_size;
size_t rc = fread (&frame_size, sizeof (frame_size), 1, file);
if (rc == 1) {
View
6 src/zsocket.c
@@ -139,8 +139,8 @@ zsocket_disconnect (void *self, const char *format, ...)
}
// --------------------------------------------------------------------------
-// Poll for input events on the socket. Returns TRUE if there is input
-// ready on the socket, else FALSE.
+// Poll for input events on the socket. Returns true if there is input
+// ready on the socket, else false.
bool
zsocket_poll (void *self, int msecs)
@@ -206,7 +206,7 @@ zsocket_test (bool verbose)
int port = zsocket_bind (writer, "tcp://%s:*", interf);
assert (port >= ZSOCKET_DYNFROM && port <= ZSOCKET_DYNTO);
- assert (zsocket_poll (writer, 100) == FALSE);
+ assert (zsocket_poll (writer, 100) == false);
rc = zsocket_connect (reader, "txp://%s:%d", domain, service);
assert (rc == -1);
View
4 src/zstr.c
@@ -152,7 +152,7 @@ zstr_sendf (void *zocket, const char *format, ...)
va_list argptr;
va_start (argptr, format);
- int rc = s_zstr_sendf_impl (zocket, FALSE, format, argptr);
+ int rc = s_zstr_sendf_impl (zocket, false, format, argptr);
va_end (argptr);
return rc;
@@ -168,7 +168,7 @@ zstr_sendfm (void *zocket, const char *format, ...)
va_list argptr;
va_start (argptr, format);
- int rc = s_zstr_sendf_impl (zocket, TRUE, format, argptr);
+ int rc = s_zstr_sendf_impl (zocket, true, format, argptr);
va_end (argptr);
return rc;
Something went wrong with that request. Please try again.