gtp/queue/queue_seqdel(): fix element check which always was true

Iterate over all member until the correct member found, remove it
and rearrange the seqnext pointer
---
 gtp/queue.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

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