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

historical

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

-- 
- Neels Hofmeyr <nhofmeyr at sysmocom.de>          http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschäftsführer / Managing Directors: Holger Freyther, Harald Welte
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.osmocom.org/pipermail/osmocom-net-gprs/attachments/20160601/fdddd7f6/attachment.bin>