Module: sip-router Branch: master Commit: 67172188fa23112fa449cf60d790bc84d02fed28 URL: http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=6717218… Author: Miklos Tirpak &lt;miklos(a)iptel.org&gt; Committer: Miklos Tirpak &lt;miklos(a)iptel.org&gt; Date: Thu Sep 15 17:05:36 2011 +0200 cfg framework: fix the freeing of the replaced strings The replaced strings and the memory block of the replaced group instances cannot be freed when the old configuration block is freed. There might be a child process using an even older configuration that references to the same string value or to the same group instance that is beeing replaced. Hence, as long as there is any child process with an older configuration, the replaced strings cannot be freed. The fix is to link the replaced strings to the per-child process callback list instead of the old cfg block. When the last child process updates its configuration, it also frees the old string values. --- cfg/cfg_ctx.c | 4 ++-- cfg/cfg_struct.c | 33 +++++++++++++++++++++++++-------- cfg/cfg_struct.h | 38 ++++++++++++++++++++++---------------- 3 files changed, 49 insertions(+), 26 deletions(-) diff --git a/cfg/cfg_ctx.c b/cfg/cfg_ctx.c index 304a8c8..e0a18e7 100644 --- a/cfg/cfg_ctx.c +++ b/cfg/cfg_ctx.c @@ -649,7 +649,7 @@ error: if (cfg_shmized) CFG_WRITER_UNLOCK(); if (block) cfg_block_free(block); if (new_array) shm_free(new_array); - if (child_cb) cfg_child_cb_free(child_cb); + if (child_cb) cfg_child_cb_free_list(child_cb); if (replaced) shm_free(replaced); error0: @@ -1235,7 +1235,7 @@ error: error0: CFG_CTX_UNLOCK(ctx); - if (child_cb_first) cfg_child_cb_free(child_cb_first); + if (child_cb_first) cfg_child_cb_free_list(child_cb_first); if (replaced) shm_free(replaced); return -1; diff --git a/cfg/cfg_struct.c b/cfg/cfg_struct.c index e07098a..c2e6520 100644 --- a/cfg/cfg_struct.c +++ b/cfg/cfg_struct.c @@ -401,7 +401,7 @@ void cfg_destroy(void) cfg_free_selects(); if (cfg_child_cb_first) { - if (*cfg_child_cb_first) cfg_child_cb_free(*cfg_child_cb_first); + if (*cfg_child_cb_first) cfg_child_cb_free_list(*cfg_child_cb_first); shm_free(cfg_child_cb_first); cfg_child_cb_first = NULL; } @@ -533,7 +533,7 @@ void cfg_child_destroy(void) if (*cfg_child_cb_first == prev_cb) { /* yes, this process was blocking the deletion */ *cfg_child_cb_first = cfg_child_cb; - shm_free(prev_cb); + cfg_child_cb_free_item(prev_cb); } } else { /* no need to continue, because there is at least @@ -793,6 +793,26 @@ void cfg_install_global(cfg_block_t *block, void **replaced, CFG_REF(block); + if (replaced) { + /* The replaced array is specified, it has to be linked to the child cb structure. + * The last child process processing this structure will free the old strings and the array. */ + if (cb_first) { + cb_first->replaced = replaced; + } else { + /* At least one child cb structure is needed. */ + cb_first = cfg_child_cb_new(NULL, NULL, NULL, 0 /* gname, name, cb, type */); + if (cb_first) { + cb_last = cb_first; + cb_first->replaced = replaced; + } else { + LOG(L_ERR, "ERROR: cfg_install_global(): not enough shm memory\n"); + /* Nothing more can be done here, the replaced strings are still needed, + * they cannot be freed at this moment. + */ + } + } + } + CFG_LOCK(); old_cfg = *cfg_global; @@ -803,11 +823,8 @@ void cfg_install_global(cfg_block_t *block, void **replaced, CFG_UNLOCK(); - if (old_cfg) { - if (replaced) (old_cfg)->replaced = replaced; + if (old_cfg) CFG_UNREF(old_cfg); - } - } /* creates a structure for a per-child process callback */ @@ -854,7 +871,7 @@ cfg_child_cb_t *cfg_child_cb_new(str *gname, str *name, } /* free the memory allocated for a child cb list */ -void cfg_child_cb_free(cfg_child_cb_t *child_cb_first) +void cfg_child_cb_free_list(cfg_child_cb_t *child_cb_first) { cfg_child_cb_t *cb, *cb_next; @@ -863,7 +880,7 @@ void cfg_child_cb_free(cfg_child_cb_t *child_cb_first) cb = cb_next ) { cb_next = cb->next; - shm_free(cb); + cfg_child_cb_free_item(cb); } } diff --git a/cfg/cfg_struct.h b/cfg/cfg_struct.h index dca39a0..505c0b4 100644 --- a/cfg/cfg_struct.h +++ b/cfg/cfg_struct.h @@ -136,10 +136,6 @@ typedef struct _cfg_block { atomic_t refcnt; /*!< reference counter, the block is automatically deleted when it reaches 0 */ - void **replaced; /*!< set of the strings and other memory segments - that must be freed - together with the block. The content depends - on the block that replaces this one */ unsigned char vars[1]; /*!< blob that contains the values */ } cfg_block_t; @@ -159,7 +155,12 @@ typedef struct _cfg_child_cb { * <=0 the cb no longer needs to be executed */ str gname, name; /*!< name of the variable that has changed */ - cfg_on_set_child cb; /*!< callback function that has to be called */ + cfg_on_set_child cb; /*!< callback function that has to be called */ + void **replaced; /*!< set of strings and other memory segments + that must be freed together with this structure. + The content depends on the new config block. + This makes sure that the replaced strings are freed + after all the child processes release the old configuration. */ struct _cfg_child_cb *next; } cfg_child_cb_t; @@ -274,20 +275,23 @@ void cfg_set_group(cfg_group_t *group, /* copy the variables to shm mem */ int cfg_shmize(void); -/* free the memory of a config block */ -static inline void cfg_block_free(cfg_block_t *block) +/* free the memory of a child cb structure */ +static inline void cfg_child_cb_free_item(cfg_child_cb_t *cb) { int i; /* free the changed variables */ - if (block->replaced) { - for (i=0; block->replaced[i]; i++) - shm_free(block->replaced[i]); - shm_free(block->replaced); + if (cb->replaced) { + for (i=0; cb->replaced[i]; i++) + shm_free(cb->replaced[i]); + shm_free(cb->replaced); } - shm_free(block); + shm_free(cb); } +#define cfg_block_free(block) \ + shm_free(block) + /* Move the group handle to the specified group instance pointed by dst_ginst. * src_ginst shall point to the active group instance. * Both parameters can be NULL meaning that the src/dst config is the default, @@ -369,13 +373,15 @@ static inline void cfg_update_local(int no_cbs) /* yes, this process was blocking the deletion */ *cfg_child_cb_first = cfg_child_cb; CFG_UNLOCK(); - shm_free(prev_cb); + cfg_child_cb_free_item(prev_cb); } else { CFG_UNLOCK(); } } - if (atomic_add(&cfg_child_cb->cb_count, -1) >= 0) /* the new value is returned - by atomic_add() */ + if (cfg_child_cb->cb + && (atomic_add(&cfg_child_cb->cb_count, -1) >= 0) /* the new value is returned + by atomic_add() */ + ) /* execute the callback */ cfg_child_cb->cb(&cfg_child_cb->gname, &cfg_child_cb->name); /* else the callback no longer needs to be executed */ @@ -500,7 +506,7 @@ cfg_child_cb_t *cfg_child_cb_new(str *gname, str *name, unsigned int type); /* free the memory allocated for a child cb list */ -void cfg_child_cb_free(cfg_child_cb_t *child_cb_first); +void cfg_child_cb_free_list(cfg_child_cb_t *child_cb_first); /* Allocate memory for a new additional variable * and link it to a configuration group.