Better refcount locking to avoid use after free
Under some conditions it could be happen that on a freeing capi object a other thread still get a reference and then it use the already freed object. To avoid this, we do 2 things, we only take a refernce if get_obj() succeed, it do not longer succeed if the object is already cleaned. We also force scheduling after releaseing the lock before really freeing the object - so the waiting thread will not get a new reference.
This commit is contained in:
Karsten Keil
parent
d9a97bb810
commit
fc757c58dc
|
|
@ -103,8 +103,12 @@ int register_lController(struct mApplication *appl, struct lController *lc)
|
|||
eprint("%s: controller idx %d ID:%d\already registered\n", CAPIobjIDstr(&appl->cobj), i, lc->cobj.id);
|
||||
return -EBUSY;
|
||||
}
|
||||
appl->lcl[i] = lc;
|
||||
get_cobj(&lc->cobj);
|
||||
if (get_cobj(&lc->cobj)) {
|
||||
appl->lcl[i] = lc;
|
||||
} else {
|
||||
eprint("%s: controller idx %d cannot get controller object %s\n", CAPIobjIDstr(&appl->cobj), i, CAPIobjIDstr(&lc->cobj));
|
||||
return -EINVAL;
|
||||
}
|
||||
return 0;
|
||||
}
|
||||
|
||||
|
|
@ -290,8 +294,12 @@ struct lController *get_lController(struct mApplication *appl, unsigned int cont
|
|||
wprint("%s: wrong controller id %d (max %d)\n", CAPIobjIDstr(&appl->cobj), cont, mI_ControllerCount);
|
||||
lc = NULL;
|
||||
}
|
||||
if (lc)
|
||||
get_cobj(&lc->cobj);
|
||||
if (lc) {
|
||||
if (!get_cobj(&lc->cobj)) {
|
||||
wprint("%s: cannot get controller object %s\n", CAPIobjIDstr(&appl->cobj), CAPIobjIDstr(&lc->cobj));
|
||||
lc = NULL;
|
||||
}
|
||||
}
|
||||
return lc;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -132,6 +132,7 @@ void CAPIobj_exit(void)
|
|||
while (co) {
|
||||
cn = co->nextD;
|
||||
eprint("%s: uid=%i refcnt %d in dangling list - freeing now\n", CAPIobjIDstr(co), co->uid, co->refcnt);
|
||||
co->cleaned = 1;
|
||||
cobj_free(co);
|
||||
co = cn;
|
||||
}
|
||||
|
|
@ -357,9 +358,12 @@ struct mCAPIobj *__get_cobj(struct mCAPIobj *co, const char *file, int lineno)
|
|||
struct mCAPIobj *get_cobj(struct mCAPIobj *co)
|
||||
#endif
|
||||
{
|
||||
struct mCAPIobj *p;
|
||||
|
||||
if (co) {
|
||||
if (co->parent) {
|
||||
pthread_rwlock_wrlock(&co->parent->lock);
|
||||
p = co->parent;
|
||||
if (p) {
|
||||
pthread_rwlock_wrlock(&p->lock);
|
||||
} else {
|
||||
if (co->type != Cot_Root) { /* has no parent */
|
||||
cobj_err("%s: parent not assigned\n", CAPIobjIDstr(co));
|
||||
|
|
@ -367,12 +371,17 @@ struct mCAPIobj *get_cobj(struct mCAPIobj *co)
|
|||
}
|
||||
pthread_mutex_lock(&rootLock);
|
||||
}
|
||||
if (co->freeing)
|
||||
cobj_err("Currently freeing %s refcnt: %d\n", CAPIobjIDstr(co), co->refcnt);
|
||||
co->refcnt++;
|
||||
coref_dbg("%s\n", CAPIobjIDstr(co));
|
||||
if (co->parent)
|
||||
pthread_rwlock_unlock(&co->parent->lock);
|
||||
if (co->cleaned) {
|
||||
cobj_warn("%s: pending free detected - do not get object\n", CAPIobjIDstr(co));
|
||||
co = NULL;
|
||||
} else {
|
||||
if (co->freeing)
|
||||
cobj_err("Currently freeing %s refcnt: %d\n", CAPIobjIDstr(co), co->refcnt);
|
||||
co->refcnt++;
|
||||
coref_dbg("%s\n", CAPIobjIDstr(co));
|
||||
}
|
||||
if (p)
|
||||
pthread_rwlock_unlock(&p->lock);
|
||||
else
|
||||
pthread_mutex_unlock(&rootLock);
|
||||
} else
|
||||
|
|
@ -403,6 +412,10 @@ int put_cobj(struct mCAPIobj *co)
|
|||
ret = co->refcnt;
|
||||
if (co->cleaned && co->refcnt <= 0) { /* last ref */
|
||||
pthread_rwlock_unlock(&p->lock);
|
||||
/* OK not perfect but scheduling here should prevent us from access of freed memory, if a other thread
|
||||
still pending on the lock - a cleaned object is not longer listed and do not return success in get_obj()
|
||||
so getting a new reference should not happen after this point */
|
||||
sched_yield();
|
||||
cobj_free(co);
|
||||
} else {
|
||||
pthread_rwlock_unlock(&p->lock);
|
||||
|
|
@ -447,8 +460,13 @@ struct mCAPIobj *get_next_cobj(struct mCAPIobj *parent, struct mCAPIobj *cur)
|
|||
else
|
||||
next = parent->listhead;
|
||||
if (next) {
|
||||
next->refcnt++;
|
||||
coref_dbg("%s: next %s\n", CAPIobjIDstr(cur), CAPIobjIDstr(next));
|
||||
if (next->cleaned) {
|
||||
cobj_warn("%s: pending free detected - do not get next\n", CAPIobjIDstr(next));
|
||||
next = NULL;
|
||||
} else {
|
||||
next->refcnt++;
|
||||
coref_dbg("%s: next %s\n", CAPIobjIDstr(cur), CAPIobjIDstr(next));
|
||||
}
|
||||
}
|
||||
pthread_rwlock_unlock(&parent->lock);
|
||||
if (parent->freeing)
|
||||
|
|
|
|||
|
|
@ -1141,6 +1141,16 @@ int OpenBInstance(struct BInstance *bi, struct lPLCI *lp)
|
|||
return ret;
|
||||
}
|
||||
|
||||
if (get_cobj(&lp->cobj)) {
|
||||
bi->lp = lp;
|
||||
} else {
|
||||
bi->lp = NULL;
|
||||
ret = -EINVAL;
|
||||
wprint("Cannot get logical controller object\n");
|
||||
close(sk);
|
||||
return ret;
|
||||
}
|
||||
|
||||
switch (lp->btype) {
|
||||
case BType_Direct:
|
||||
bi->from_down = recvBdirect;
|
||||
|
|
@ -1160,6 +1170,8 @@ int OpenBInstance(struct BInstance *bi, struct lPLCI *lp)
|
|||
eprint("Error unnkown BType %d\n", lp->btype);
|
||||
dprint(MIDEBUG_CONTROLLER, "close socket bi[%d]->fd %d\n", bi->nr, sk);
|
||||
close(sk);
|
||||
bi->lp = NULL;
|
||||
put_cobj(&lp->cobj);
|
||||
return -EINVAL;
|
||||
}
|
||||
bi->type = lp->btype;
|
||||
|
|
@ -1181,11 +1193,11 @@ int OpenBInstance(struct BInstance *bi, struct lPLCI *lp)
|
|||
bi->from_down = dummy_btrans;
|
||||
bi->from_up = dummy_btrans;
|
||||
bi->type = BType_None;
|
||||
bi->lp = NULL;
|
||||
put_cobj(&lp->cobj);
|
||||
} else {
|
||||
bi->closed = 0;
|
||||
bi->fd = sk;
|
||||
bi->lp = lp;
|
||||
get_cobj(&lp->cobj);
|
||||
bi->org_rx_min = MISDN_CTRL_RX_SIZE_IGNORE;
|
||||
bi->rx_min = MISDN_CTRL_RX_SIZE_IGNORE;
|
||||
bi->org_rx_max = MISDN_CTRL_RX_SIZE_IGNORE;
|
||||
|
|
@ -1243,8 +1255,8 @@ int OpenBInstance(struct BInstance *bi, struct lPLCI *lp)
|
|||
dprint(MIDEBUG_CONTROLLER, "close socket bi[%d]->fd %d\n", bi->nr, sk);
|
||||
close(sk);
|
||||
bi->fd = -1;
|
||||
put_cobj(&lp->cobj);
|
||||
bi->lp = NULL;
|
||||
put_cobj(&lp->cobj);
|
||||
} else {
|
||||
dprint(MIDEBUG_CONTROLLER, "Controller%d: Bchannel %d socket %d added to poll idx %d\n",
|
||||
bi->pc->profile.ncontroller, bi->nr, sk, ret);
|
||||
|
|
@ -1260,8 +1272,8 @@ int OpenBInstance(struct BInstance *bi, struct lPLCI *lp)
|
|||
dprint(MIDEBUG_CONTROLLER, "close socket bi[%d]->fd %d\n", bi->nr, sk);
|
||||
close(sk);
|
||||
bi->fd = -1;
|
||||
put_cobj(&lp->cobj);
|
||||
bi->lp = NULL;
|
||||
put_cobj(&lp->cobj);
|
||||
} else {
|
||||
ret = 0;
|
||||
bi->UpId = 0;
|
||||
|
|
@ -1274,8 +1286,8 @@ int OpenBInstance(struct BInstance *bi, struct lPLCI *lp)
|
|||
dprint(MIDEBUG_CONTROLLER, "close socket bi[%d]->fd %d\n", bi->nr, sk);
|
||||
close(sk);
|
||||
bi->fd = -1;
|
||||
put_cobj(&lp->cobj);
|
||||
bi->lp = NULL;
|
||||
put_cobj(&lp->cobj);
|
||||
return ret;
|
||||
} else {
|
||||
bi->pfd[2].fd = bi->tty;
|
||||
|
|
@ -1287,8 +1299,8 @@ int OpenBInstance(struct BInstance *bi, struct lPLCI *lp)
|
|||
dprint(MIDEBUG_CONTROLLER, "close socket bi[%d]->fd %d\n", bi->nr, sk);
|
||||
close(sk);
|
||||
bi->fd = -1;
|
||||
put_cobj(&lp->cobj);
|
||||
bi->lp = NULL;
|
||||
put_cobj(&lp->cobj);
|
||||
} else {
|
||||
ret = 0;
|
||||
bi->UpId = 0;
|
||||
|
|
|
|||
|
|
@ -128,14 +128,19 @@ struct lController *addlController(struct mApplication *app, struct pController
|
|||
lc = calloc(1, sizeof(*lc));
|
||||
if (lc) {
|
||||
lc->cobj.id2 = app->cobj.id2;
|
||||
if (!get_cobj(&app->cobj)) {
|
||||
eprint("Cannot get application object\n");
|
||||
free(lc);
|
||||
return NULL;
|
||||
}
|
||||
ret = init_cobj_registered(&lc->cobj, &pc->cobjLC, Cot_lController, 0x0000ff);
|
||||
if (ret) {
|
||||
eprint("Controller%d: Application %d - cannot init\n", pc->profile.ncontroller, app->cobj.id2);
|
||||
put_cobj(&app->cobj);
|
||||
free(lc);
|
||||
lc = NULL;
|
||||
} else {
|
||||
lc->Appl = app;
|
||||
get_cobj(&app->cobj);
|
||||
lc->listen_m.fsm = &listen_fsm;
|
||||
lc->listen_m.state = ST_LISTEN_L_0;
|
||||
lc->listen_m.debug = MIDEBUG_CONTROLLER & mI_debug_mask;
|
||||
|
|
@ -146,10 +151,13 @@ struct lController *addlController(struct mApplication *app, struct pController
|
|||
lc->CIPmask2 = 0;
|
||||
ret = register_lController(app, lc);
|
||||
if (ret) {
|
||||
lc->Appl = NULL;
|
||||
put_cobj(&app->cobj);
|
||||
eprint("Controller%d: - cannot register LC on Application %d - %s\n", pc->profile.ncontroller,
|
||||
app->cobj.id2, strerror(-ret));
|
||||
free(lc);
|
||||
lc->cobj.cleaned = 1;
|
||||
delist_cobj(&lc->cobj);
|
||||
put_cobj(&lc->cobj);
|
||||
lc = NULL;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1747,10 +1747,24 @@ int lPLCICreate(struct lPLCI **lpp, struct lController *lc, struct mPLCI *plci,
|
|||
return ret;
|
||||
}
|
||||
lp->cipmask = cipmask;
|
||||
lp->lc = lc;
|
||||
get_cobj(&lc->cobj);
|
||||
lp->Appl = lc->Appl;
|
||||
get_cobj(&lc->Appl->cobj);
|
||||
if (get_cobj(&lc->cobj)) {
|
||||
lp->lc = lc;
|
||||
} else {
|
||||
wprint("%s: Cannot get lController object\n", CAPIobjIDstr(&lc->cobj));
|
||||
lp->cobj.cleaned = 1;
|
||||
delist_cobj(&lp->cobj);
|
||||
put_cobj(&lp->cobj); /* will cleanup and free too */
|
||||
return -EINVAL;
|
||||
}
|
||||
if (get_cobj(&lc->Appl->cobj)) {
|
||||
lp->Appl = lc->Appl;
|
||||
} else {
|
||||
wprint("%s: Cannot get application object\n", CAPIobjIDstr(&lc->Appl->cobj));
|
||||
lp->cobj.cleaned = 1;
|
||||
delist_cobj(&lp->cobj);
|
||||
put_cobj(&lp->cobj); /* will cleanup and free too */
|
||||
return -EINVAL;
|
||||
}
|
||||
if (plci->pc->profile.goptions & 0x0008) {
|
||||
/* DTMF */
|
||||
lp->l1dtmf = 1;
|
||||
|
|
@ -1762,8 +1776,15 @@ int lPLCICreate(struct lPLCI **lpp, struct lController *lc, struct mPLCI *plci,
|
|||
lp->plci_m.printdebug = lPLCI_debug;
|
||||
lp->chid.nr = MI_CHAN_NONE;
|
||||
lp->autohangup = 1;
|
||||
init_timer(&lp->atimer, mICAPItimer_base, lp, atimer_timeout);
|
||||
get_cobj(&lp->cobj); /* timer ref */
|
||||
if (get_cobj(&lp->cobj)) { /* timer ref */
|
||||
init_timer(&lp->atimer, mICAPItimer_base, lp, atimer_timeout);
|
||||
} else {
|
||||
wprint("%s: Cannot get lplci object for timer ref\n", CAPIobjIDstr(&lp->cobj));
|
||||
lp->cobj.cleaned = 1;
|
||||
delist_cobj(&lp->cobj);
|
||||
put_cobj(&lp->cobj); /* will cleanup and free too */
|
||||
return -EINVAL;
|
||||
}
|
||||
*lpp = lp;
|
||||
return 0;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -133,19 +133,26 @@ struct mCAPIobj {
|
|||
#endif
|
||||
};
|
||||
|
||||
#if __GNUC_PREREQ (3,4)
|
||||
# define __WUR __attribute__ ((__warn_unused_result__))
|
||||
#else
|
||||
# define __WUR
|
||||
#endif
|
||||
|
||||
|
||||
#ifdef MISDN_CAPI_REFCOUNT_DEBUG
|
||||
struct mCAPIobj *__get_cobj(struct mCAPIobj *, const char *, int);
|
||||
struct mCAPIobj *__get_cobj(struct mCAPIobj *, const char *, int) __WUR;
|
||||
int __put_cobj(struct mCAPIobj *, const char *, int);
|
||||
struct mCAPIobj *__get_next_cobj(struct mCAPIobj *, struct mCAPIobj *, const char *, int);
|
||||
struct mCAPIobj *__get_next_cobj(struct mCAPIobj *, struct mCAPIobj *, const char *, int) __WUR;
|
||||
int __delist_cobj(struct mCAPIobj *, const char *, int);
|
||||
#define get_cobj(co) __get_cobj(co, __FILE__, __LINE__)
|
||||
#define put_cobj(co) __put_cobj(co, __FILE__, __LINE__)
|
||||
#define get_next_cobj(pa, co) __get_next_cobj(pa, co, __FILE__, __LINE__)
|
||||
#define delist_cobj(co) __delist_cobj(co, __FILE__, __LINE__)
|
||||
#else
|
||||
struct mCAPIobj *get_cobj(struct mCAPIobj *);
|
||||
struct mCAPIobj *get_cobj(struct mCAPIobj *) __WUR;
|
||||
int put_cobj(struct mCAPIobj *);
|
||||
struct mCAPIobj *get_next_cobj(struct mCAPIobj *, struct mCAPIobj *);
|
||||
struct mCAPIobj *get_next_cobj(struct mCAPIobj *, struct mCAPIobj *) __WUR;
|
||||
int delist_cobj(struct mCAPIobj *);
|
||||
#endif
|
||||
|
||||
|
|
|
|||
|
|
@ -640,6 +640,7 @@ struct mNCCI *ncciCreate(struct lPLCI *lp)
|
|||
{
|
||||
struct mNCCI *nc;
|
||||
int err;
|
||||
struct mApplication *appl = NULL;
|
||||
|
||||
nc = calloc(1, sizeof(*nc));
|
||||
if (!nc) {
|
||||
|
|
@ -647,6 +648,15 @@ struct mNCCI *ncciCreate(struct lPLCI *lp)
|
|||
return NULL;
|
||||
}
|
||||
nc->cobj.id2 = lp->cobj.id2;
|
||||
if (lp->Appl) {
|
||||
if (get_cobj(&lp->Appl->cobj)) {
|
||||
appl = lp->Appl;
|
||||
} else {
|
||||
wprint("Cannot get application object\n");
|
||||
free(nc);
|
||||
return NULL;
|
||||
}
|
||||
}
|
||||
err = init_cobj_registered(&nc->cobj, &lp->cobj, Cot_NCCI, 0x01ffff);
|
||||
if (!err) {
|
||||
dprint(MIDEBUG_NCCI, "NCCI%06x: will be created now\n", nc->cobj.id);
|
||||
|
|
@ -654,9 +664,7 @@ struct mNCCI *ncciCreate(struct lPLCI *lp)
|
|||
nc->ncci_m.debug = MIDEBUG_NCCI & mI_debug_mask;
|
||||
nc->ncci_m.userdata = nc;
|
||||
nc->ncci_m.printdebug = ncci_debug;
|
||||
nc->appl = lp->Appl;
|
||||
if (nc->appl)
|
||||
get_cobj(&nc->appl->cobj);
|
||||
nc->appl = appl;
|
||||
nc->BIlink = lp->BIlink;
|
||||
nc->window = lp->Appl->MaxB3Blk;
|
||||
pthread_mutex_init(&nc->lock, NULL);
|
||||
|
|
@ -710,6 +718,8 @@ struct mNCCI *ncciCreate(struct lPLCI *lp)
|
|||
nc->osize = 256;
|
||||
dprint(MIDEBUG_NCCI, "%s: created\n", CAPIobjIDstr(&nc->cobj));
|
||||
} else {
|
||||
if (appl)
|
||||
put_cobj(&appl->cobj);
|
||||
free(nc);
|
||||
nc = NULL;
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue