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