On Mon, Mar 11, 2013 at 01:07:50AM +0100, Holger Hans Peter Freyther wrote:
> Hi,
>
> I was discussing this crash[1] with Jan at the 29C3 and recently in
> Iceland. On top of that Katarina pointed me to the best practises[2]
> of talloc. In general I disagree with them[3] but they provide a nice
> solution for the SGSN/MSGB ownership issue.
Hi,
so attached is a proof of concept. This has only been compile tested.
In theory the code should now:
1.) Place the msgb in the write_queue when it will be stolen back into
the msgb context (it could be moved into a write queue context)
2.) Delete the msgb in case of error on the way down there..
3.) Catch all and the msgb is still in the local_ctx and we just free it.
Please comment.
holger
diff --git a/src/write_queue.c b/src/write_queue.c
index cef40f8..3f587dc 100644
--- a/src/write_queue.c
+++ b/src/write_queue.c
@@ -22,6 +22,9 @@
*/
#include <osmocom/core/write_queue.h>
+#include <osmocom/core/talloc.h>
+
+extern void *tall_msgb_ctx;
/*! \addtogroup write_queue
* @{
@@ -92,6 +95,9 @@ int osmo_wqueue_enqueue(struct osmo_wqueue *queue, struct msgb *data)
// if (queue->current_length + 1 >= queue->max_length)
// LOGP(DMSC, LOGL_ERROR, "The queue is full. Dropping not yet implemented.\n");
+ /* take ownership and potentially move it back to the msgb ctx */
+ talloc_steal(tall_msgb_ctx, data);
+
++queue->current_length;
msgb_enqueue(&queue->msg_queue, data);
queue->bfd.when |= BSC_FD_WRITE;
diff --git a/openbsc/src/gprs/gprs_sndcp.c b/openbsc/src/gprs/gprs_sndcp.c
index 853f8db..d3112e7 100644
--- a/openbsc/src/gprs/gprs_sndcp.c
+++ b/openbsc/src/gprs/gprs_sndcp.c
@@ -353,16 +353,24 @@ static int sndcp_send_ud_frag(struct sndcp_frag_state *fs)
struct sndcp_comp_hdr *scomph;
struct sndcp_udata_hdr *suh;
struct msgb *fmsg;
+ void *local_ctx;
unsigned int max_payload_len;
unsigned int len;
uint8_t *data;
int rc, more;
+ local_ctx = talloc_new(NULL);
+ if (!local_ctx)
+ return -ENOMEM;
+
fmsg = msgb_alloc_headroom(fs->sne->lle->params.n201_u+256, 128,
"SNDCP Frag");
if (!fmsg)
return -ENOMEM;
+ /* now steal into the context */
+ talloc_steal(local_ctx, fmsg);
+
/* make sure lower layers route the fragment like the original */
msgb_tlli(fmsg) = msgb_tlli(fs->msg);
msgb_bvci(fmsg) = msgb_bvci(fs->msg);
@@ -418,9 +426,9 @@ static int sndcp_send_ud_frag(struct sndcp_frag_state *fs)
sch->more = more;
rc = gprs_llc_tx_ui(fmsg, lle->sapi, 0, fs->mmcontext);
+ talloc_free(local_ctx);
if (rc < 0) {
/* abort in case of error, do not advance frag_nr / next_byte */
- msgb_free(fmsg);
return rc;
}
