Message ID | 20160531124238.29585-1-lynxis@fe80.eu |
---|---|
State | New |
Series | "gtp/queue/queue_seqdel(): fix element check which always was true" |
Headers | show |
diff --git a/gtp/queue.c b/gtp/queue.c index 5b4d849..fbfa1ec 100644 --- a/gtp/queue.c +++ b/gtp/queue.c @@ -105,8 +105,7 @@ static int queue_seqdel(struct queue_t *queue, struct qmsg_t *qmsg) printf("Begin queue_seqdel seq = %d\n", (int)qmsg->seq); for (qmsg2 = queue->hashseq[hash]; qmsg2; qmsg2 = qmsg2->seqnext) { - /* FIXME: this is always true !?! */ - if (qmsg == qmsg) { + if (qmsg == qmsg2) { if (!qmsg_prev) queue->hashseq[hash] = qmsg2->seqnext; else
On Tue, May 31, 2016 at 02:42:38PM +0200, Alexander Couzens wrote: > Iterate over all member until the correct member found, remove it > and rearrange the seqnext pointer Not sure why you put this ^ in the commit log? Sounds like an in-code comment instead. The commit is only about what you wrote in the subject. > --- > gtp/queue.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/gtp/queue.c b/gtp/queue.c > index 5b4d849..fbfa1ec 100644 > --- a/gtp/queue.c > +++ b/gtp/queue.c > @@ -105,8 +105,7 @@ static int queue_seqdel(struct queue_t *queue, struct qmsg_t *qmsg) > printf("Begin queue_seqdel seq = %d\n", (int)qmsg->seq); > > for (qmsg2 = queue->hashseq[hash]; qmsg2; qmsg2 = qmsg2->seqnext) { > - /* FIXME: this is always true !?! */ > - if (qmsg == qmsg) { > + if (qmsg == qmsg2) { Wow, this is really bad. This fix looks really obvious and I'm tempted to merge right away. Still, have you tested whether it works? We should definitely have unit tests for these functions! Let's understand how openggsn can work when the function that removes an item from a queue is fundamentally broken. queue_seqdel() always removes the first item from the queue and returns 0. Maybe, most of the time, the first item is coincidentally the one that openggsn intends to remove? queue_seqdel() is used by queue_freemsg() and in turn queue_freemsg_seq(). queue_freemsg() is supposed to delete a specific item from the queue. Its only caller (besides queue_freemsg_seq()) is gtp_retrans(). All of the invocations of queue_freemsg() are preceded by queue_getfirst(). Always the first item! queue_freemsg_seq() finds a queue item for a *given peer* and pops it from the queue (IMHO function name fail). So here it seems if we have only one peer, the first queue item would always be correct, and hence queue_seqdel() would correctly pop the first item from the queue. But also, the seq is used to look up the item to be removed, so I guess most of the time the next seq is coincidentally the first item, or the packets would be retransmitted out-of-sequence. Anyway, openggsn should break badly as soon as we have more than one peer (that's an SGSN I presume). How would it break? queue_freemsg_seq()'s only caller is gtp_conf(), which apparently does: "Remove signalling packet from retransmission queue. return 0 on success, EOF if packet was not found" (IMHO again function name fail). So it seems when there are retransmissions pending for more than one peer, openggsn would potentially transmit a given peer's queued messages to another, wrong peer. Let's have unit tests! @Harald: any preferences on who should spend time on it when? Just merge the fix (without testing) and carry on with our other tasks? Added http://osmocom.org/issues/1740 ~Neels
Hi Neels, On Wed, Jun 01, 2016 at 01:51:38PM +0200, Neels Hofmeyr wrote: > Let's have unit tests! Agreed, but I would really hope we can put new efforts into a different (cleaner, more osmocom style) GTP implementation, rather than implementing comprehensive unit tests for OpenGGSN's libgtp. > @Harald: any preferences on who should spend time on it when? Just merge the > fix (without testing) and carry on with our other tasks? I think at the moment we (at sysmocom) unfortunately have lots of higher priority items to work on, some of them are already delayed by almost 6 months. So we really cannot afford to spend time on other topics until we make more progress.
On Thu, Jun 02, 2016 at 02:22:42PM +0200, Harald Welte wrote: > Hi Neels, > > On Wed, Jun 01, 2016 at 01:51:38PM +0200, Neels Hofmeyr wrote: > > Let's have unit tests! > > Agreed, but I would really hope we can put new efforts into a different > (cleaner, more osmocom style) GTP implementation, rather than > implementing comprehensive unit tests for OpenGGSN's libgtp. Hah, nice: that was my feeling exactly, but so far I assumed that a choice had been made to use openggsn instead (for other reasons than available time). > I think at the moment we (at sysmocom) unfortunately have lots of higher > priority items to work on[...] Ok; after my analysis of the surrounding code I actually feel more or less comfortable to merge the obvious fix without insisting on tests. Merged to openggsn master, with a commit message tweak containing a link to this message thread. ~Neels