Skip to content

Commit

Permalink
struct hist API: Fix possible crashes; Improve API
Browse files Browse the repository at this point in the history 
Commit a74fff1 introduced a race condition on this logic:

    lock_get(&sh->shlist->wlock);
    sh_unref_unsafe(sh);
    lock_release(&sh->shlist->wlock);

, where "sh" must no longer be read following the unref operation.
This commit fixes this issue, along with:

    * fix crash with -DSTRUCT_HIST but no -DDBG_TCPCON
    * speed optimizations: eliminate memset() operations (not needed)
    * make sh_push() more flexible (extra ref counts from outside)
    * code: hide structs, so importing struct_hist.h doesn't conflict
       with mysql.h's own "struct list_head"

(cherry picked from commit 0db9467)
  • Loading branch information
@liviuchircu
liviuchircu committed Oct 17, 2019
1 parent 51d6fa7 commit d9b0102
Show file tree
Hide file tree
Showing 3 changed files with 69 additions and 50 deletions.
69 changes: 56 additions & 13 deletions lib/dbg/struct_hist.c
View file
Expand Up @@ -22,10 +22,44 @@
#include <stdarg.h>

#include "struct_hist.h"

#include "../../mem/shm_mem.h"
#include "../../dprint.h"
#include "../../locking.h"
#include "../../pt.h"
#include "../list.h"

struct struct_hist {
void *obj;
char *obj_name;
utime_t created;
struct struct_hist_list *shlist;

int ref;

struct struct_hist_action *actions;
int len;
int max_len;
int flush_offset;

gen_lock_t wlock;
int auto_logging;

struct list_head list;
};

struct struct_hist_list {
char *obj_name;

struct list_head objects;
int len;
int win_sz;
long long total_obj;
int auto_logging;
int init_actions_sz;

gen_lock_t wlock;
};

static inline const char *verb2str(enum struct_hist_verb verb)
{
Expand All @@ -45,8 +79,8 @@ static inline const char *verb2str(enum struct_hist_verb verb)
static void sh_unref_unsafe(struct struct_hist *sh);
static void sh_free(struct struct_hist *sh);

struct struct_hist_list *shl_init(char *obj_name, int window_size,
int auto_logging)
struct struct_hist_list *_shl_init(char *obj_name, int window_size,
int auto_logging, int init_actions_sz)
{
struct struct_hist_list *shl;

Expand All @@ -62,6 +96,7 @@ struct struct_hist_list *shl_init(char *obj_name, int window_size,
shl->win_sz = window_size;
shl->obj_name = obj_name;
shl->auto_logging = !!auto_logging;
shl->init_actions_sz = init_actions_sz;

return shl;
}
Expand All @@ -71,6 +106,9 @@ void shl_destroy(struct struct_hist_list *shl)
struct list_head *el, *next;
struct struct_hist *sh;

if (!shl)
return;

list_for_each_safe(el, next, &shl->objects) {
sh = list_entry(el, struct struct_hist, list);
sh_free(sh);
Expand All @@ -79,35 +117,38 @@ void shl_destroy(struct struct_hist_list *shl)
shm_free(shl);
}

struct struct_hist *sh_push(void *obj, struct struct_hist_list *list)
struct struct_hist *_sh_push(void *obj, struct struct_hist_list *list, int refs)
{
struct struct_hist *sh, *last;

if (!obj)
if (!obj || !list)
return NULL;

sh = shm_malloc(sizeof *sh);
if (!sh) {
LM_ERR("oom\n");
return NULL;
}
memset(sh, 0, sizeof *sh);
/* CAREFUL: sh is not memset, for speed reasons! */

sh->actions = shm_malloc(ACTIONS_SIZE * sizeof *sh->actions);
sh->actions = shm_malloc(list->init_actions_sz * sizeof *sh->actions);
if (!sh->actions) {
LM_ERR("oom2\n");
shm_free(sh);
return NULL;
}
memset(sh->actions, 0, ACTIONS_SIZE * sizeof *sh->actions);
/* CAREFUL: sh->actions is not memset, for speed reasons! */

sh->obj = obj;
sh->obj_name = list->obj_name;
sh->shlist = list;
sh->created = get_uticks();
sh->ref = 2; /* one for "list", one for "return sh;" */
sh->max_len = ACTIONS_SIZE;
sh->shlist = list;
sh->ref = 1 + refs; /* one for "list", the rest are for the caller */
sh->len = 0;
sh->max_len = list->init_actions_sz;
sh->flush_offset = 0;
sh->auto_logging = list->auto_logging;

lock_init(&sh->wlock);

lock_get(&list->wlock);
Expand Down Expand Up @@ -135,9 +176,11 @@ static void sh_free(struct struct_hist *sh)

void sh_unref(struct struct_hist *sh)
{
lock_get(&sh->shlist->wlock);
gen_lock_t *shl_lock = &sh->shlist->wlock;

lock_get(shl_lock);
sh_unref_unsafe(sh);
lock_release(&sh->shlist->wlock);
lock_release(shl_lock);
}

static void _sh_flush(struct struct_hist *sh, int do_logging)
Expand Down Expand Up @@ -216,7 +259,7 @@ int _sh_log(struct struct_hist *sh, enum struct_hist_verb verb, char *fmt, ...)
LM_ERR("oom\n");
return -1;
}
memset(&new[sh->max_len], 0, sh->max_len * sizeof *sh->actions);
/* CAREFUL: newly added actions are not memset, for speed reasons! */

sh->actions = new;
sh->max_len *= 2;
Expand Down
46 changes: 9 additions & 37 deletions lib/dbg/struct_hist.h
View file
Expand Up @@ -21,11 +21,8 @@
#ifndef __STRUCT_HIST_H__
#define __STRUCT_HIST_H__

#include "../../locking.h"
#include "../../timer.h"

#include "../list.h"

/**
* Generic struct debugging support. Especially useful for troubleshooting
* bugs related to reference counted structures, including:
Expand Down Expand Up @@ -75,41 +72,12 @@ struct struct_hist_action {
char log[MAX_SHLOG_SIZE];
};

#define ACTIONS_SIZE 5
struct struct_hist {
void *obj;
char *obj_name;
utime_t created;
struct struct_hist_list *shlist;

int ref;

struct struct_hist_action *actions;
int len;
int max_len;
int flush_offset;

gen_lock_t wlock;
int auto_logging;

struct list_head list;
};
struct struct_hist;
struct struct_hist_list;

#define FLUSH_LIMIT 2000
#define flushable(sh) (sh->len == FLUSH_LIMIT)

struct struct_hist_list {
char *obj_name;

struct list_head objects;
int len;
int win_sz;
long long total_obj;
int auto_logging;

gen_lock_t wlock;
};

#ifndef DBG_STRUCT_HIST
#define shl_init(...) NULL
#define shl_destroy(...)
Expand All @@ -127,12 +95,14 @@ struct struct_hist_list {
* @obj_name: A name for the structs which will be troubleshooted
* @window_size: (gliding window) - the max number of retained histories
* @auto_logging: if true, each struct_hist object will log info as it grows
* @init_actions_sz: initial allocation size of each object's actions array
*
* WARNING: a "window_size" of 0 (infinite) is essentially a memory leak,
* use with caution!
*/
struct struct_hist_list *shl_init(char *obj_name, int window_size,
int auto_logging);
struct struct_hist_list *_shl_init(char *obj_name, int window_size,
int auto_logging, int init_actions_sz);
#define shl_init(nm, wsz, autolog) _shl_init(nm, wsz, autolog, 5)

/**
* Frees up the global history holder, along with all of its content
Expand All @@ -145,8 +115,10 @@ void shl_destroy(struct struct_hist_list *shl);
* @obj: The corresponding struct address. This value will be embedded into
* the history object, to allow look-ups inside gdb
* @list: global holder where this history will be pushed
* @refs: the amount of references to the new object kept by the calling code
*/
struct struct_hist *sh_push(void *obj, struct struct_hist_list *list);
struct struct_hist *_sh_push(void *obj, struct struct_hist_list *list, int refs);
#define sh_push(obj, list) _sh_push(obj, list, 1)

/**
* Unreference a history struct. Depending on whether it is still in the
Expand Down
4 changes: 4 additions & 0 deletions net/net_tcp.c
View file
Expand Up @@ -740,9 +740,11 @@ static void _tcpconn_rm(struct tcp_connection* c)
if (protos[c->type].net.conn_clean)
protos[c->type].net.conn_clean(c);

#ifdef DBG_TCPCON
sh_log(c->hist, TCP_DESTROY, "type=%d", c->type);
sh_unref(c->hist);
c->hist = NULL;
#endif

shm_free(c);
}
Expand Down Expand Up @@ -893,7 +895,9 @@ static struct tcp_connection* tcpconn_new(int sock, union sockaddr_union* su,
/* start with the default conn lifetime */
c->lifetime = get_ticks()+tcp_con_lifetime;
c->flags|=F_CONN_REMOVED|flags;
#ifdef DBG_TCPCON
c->hist = sh_push(c, con_hist);
#endif

if (protos[si->proto].net.conn_init &&
protos[si->proto].net.conn_init(c)<0) {
Expand Down

0 comments on commit d9b0102

Please sign in to comment.