gtpie_gettlv(): fix return value on specific error.

Make gtpie_gettlv() return an error if gtpie_getie() returned an error.
Previously, openggsn would fail to complain about certain missing elements.

Technically, a missing IE could be detectable from the *length value, but the
code in gtp.c relies on the return value to detect missing elements, which did
not work prior to this commit. For example:

if (gtpie_gettlv(ie, GTPIE_EUA, 0, &pdp->eua.l,
		 &pdp->eua.v, sizeof(pdp->eua.v))) {
	gsn->missing++;
	GTP_LOGPKG(LOGL_ERROR, peer, pack,
		    len, "Missing mandatory information field\n");
	return gtp_create_pdp_resp(gsn, version, pdp,
				   GTPCAUSE_MAN_IE_MISSING);
}

If an EUA were missing in this code path, openggsn would fail to issue an error
message. Since pdp and hence pdp->eua.l is initialized as all-zero, it would
probably not do much harm besides failing to issue an error.

I haven't checked all callers though.

.gitignore

@@ -2,6 +2,7 @@ Makefile
 Makefile.in
 aclocal.m4
 autom4te.cache
+compile
 config.guess
 config.h.in*
 config.h

gtp/gtp.c

@@ -305,19 +305,19 @@ static uint32_t get_tei(void *pack)
  * requires the receiving GSN to send a response, with the same information
  * as in the original response. For most messages this happens automatically:
  *
- * Echo: Automatically dublicates the original response
+ * Echo: Automatically duplicates the original response
  * Create pdp context: The SGSN may send create context request even if
- *   a context allready exist (imsi+nsapi?). This means that the reply will
-     automatically dublicate the original response. It might however have
+ *   a context already exist (imsi+nsapi?). This means that the reply will
+ *   automatically duplicate the original response. It might however have
  *   side effects in the application which is asked twice to validate
  *   the login.
- * Update pdp context: Automatically dublicates the original response???
+ * Update pdp context: Automatically duplicates the original response???
  * Delete pdp context. Automatically in gtp0, but in gtp1 will generate
  *   a nonexist reply message.
  *
  * The correct solution will be to make a queue containing response messages.
  * This queue should be checked whenever a request is received. If the 
- * response is allready in the queue that response should be transmitted.
+ * response is already in the queue that response should be transmitted.
  * It should be possible to find messages in this queue on the basis of
  * the sequence number and peer GSN IP address (The sequense number is unique
  * within each path). This need to be implemented by a hash table. Furthermore
@@ -614,7 +614,16 @@ int gtp_notification(struct gsn_t *gsn, int version,
 	return 0;
 }
 
-int gtp_dublicate(struct gsn_t *gsn, int version,
+/* Look for a message in the response queue for the given peer and sequence
+ * number seq. If found, (re-)send the response message and return 0. Otherwise
+ * return nonzero. This allows catching duplicate messages.
+ * A response message stays in the response queue until it times out.
+ * So, for any number of identical requests within that timeout period, the
+ * same packet can be resent.
+ * TODO this only checks the peer and sequence number, and does not verify that
+ * the GTP request is identical to the previous one for this peer and sequence
+ * nr: a mere peer or seq glitch can cause leaks of previous responses. */
+int gtp_duplicate(struct gsn_t *gsn, int version,
 		  struct sockaddr_in *peer, uint16_t seq)
 {
 	struct qmsg_t *qmsg;
@@ -636,6 +645,8 @@ int gtp_dublicate(struct gsn_t *gsn, int version,
 			qmsg->fd, (unsigned long)&qmsg->p, qmsg->l,
 			strerror(errno));
 	}
+
+	/* TODO: qmsg->retrans++ ? */
 	return 0;
 }
 
@@ -873,8 +884,8 @@ int gtp_echo_ind(struct gsn_t *gsn, int version, struct sockaddr_in *peer,
 		 int fd, void *pack, unsigned len)
 {
 
-	/* Check if it was a dublicate request */
-	if (!gtp_dublicate(gsn, 0, peer, get_seq(pack)))
+	/* Repeat previous response in case of duplicate request */
+	if (!gtp_duplicate(gsn, 0, peer, get_seq(pack)))
 		return 0;
 
 	/* Send off reply to request */
@@ -1258,13 +1269,15 @@ int gtp_create_pdp_ind(struct gsn_t *gsn, int version,
 	uint8_t linked_nsapi = 0;
 	struct pdp_t *linked_pdp = NULL;
 
-	if (!gtp_dublicate(gsn, version, peer, seq))
+	/* Repeat previous response in case of duplicate request */
+	if (!gtp_duplicate(gsn, version, peer, seq))
 		return 0;
 
 	pdp = &pdp_buf;
 	memset(pdp, 0, sizeof(struct pdp_t));
 
 	if (version == 0) {
+		/* TODO code dup: get_tid() */
 		uint64_t tid = be64toh(((union gtp_packet *)pack)->gtp0.h.tid);
 
 		pdp_set_imsi_nsapi(pdp, tid);
@@ -1511,7 +1524,7 @@ int gtp_create_pdp_ind(struct gsn_t *gsn, int version,
 	if (!pdp_getimsi(&pdp_old, pdp->imsi, pdp->nsapi)) {
 		/* Found old pdp with same tid. Now the voodoo begins! */
 		/* 09.60 / 29.060 allows create on existing context to "steal" */
-		/* the context which was allready established */
+		/* the context which was already established */
 		/* We check that the APN, selection mode and MSISDN is the same */
 		DEBUGP(DLGTP, "gtp_create_pdp_ind: Old context found\n");
 		if ((pdp->apn_req.l == pdp_old->apn_req.l)
@@ -1953,10 +1966,9 @@ int gtp_update_pdp_ind(struct gsn_t *gsn, int version,
 	uint64_t imsi;
 	uint8_t nsapi;
 
-	/* Is this a dublicate ? */
-	if (!gtp_dublicate(gsn, version, peer, seq)) {
-		return 0;	/* We allready send of response once */
-	}
+	/* Repeat previous response in case of duplicate request */
+	if (!gtp_duplicate(gsn, version, peer, seq))
+		return 0;
 
 	/* Decode information elements */
 	if (gtpie_decaps(ie, version, pack + hlen, len - hlen)) {
@@ -2085,7 +2097,7 @@ int gtp_update_pdp_ind(struct gsn_t *gsn, int version,
 		}
 
 		/* TEIC (conditional) */
-		/* If TEIC is not included it means that we have allready received it */
+		/* If TEIC is not included it means that we have already received it */
 		/* TODO: From 29.060 it is not clear if TEI_C MUST be included for */
 		/* all updated contexts, or only for one of the linked contexts */
 		gtpie_gettv4(ie, GTPIE_TEI_C, 0, &pdp->teic_gn);
@@ -2179,6 +2191,9 @@ int gtp_update_pdp_conf(struct gsn_t *gsn, int version,
 	if (gtp_conf(gsn, 0, peer, pack, len, &type, &cbp))
 		return EOF;
 
+	/* TODO This function is called from gtp_decaps1c() (for GTP v1) but
+	 * uses gtp0.h.flow (GTP v0 data element)
+	 */
 	/* Find the context in question */
 	if (pdp_getgtp0(&pdp, ntoh16(((union gtp_packet *)pack)->gtp0.h.flow))) {
 		gsn->err_unknownpdp++;
@@ -2192,6 +2207,9 @@ int gtp_update_pdp_conf(struct gsn_t *gsn, int version,
 	/* Register that we have received a valid teic from GGSN */
 	pdp->teic_confirmed = 1;
 
+	/* TODO This function is called from gtp_decaps1c() (for GTP v1) but
+	 * explicitly passes version 0 and GTP0_HEADER_SIZE to gtpie_decaps()
+	 */
 	/* Decode information elements */
 	if (gtpie_decaps
 	    (ie, 0, pack + GTP0_HEADER_SIZE, len - GTP0_HEADER_SIZE)) {
@@ -2424,10 +2442,9 @@ int gtp_delete_pdp_ind(struct gsn_t *gsn, int version,
 	int n;
 	int count = 0;
 
-	/* Is this a dublicate ? */
-	if (!gtp_dublicate(gsn, version, peer, seq)) {
-		return 0;	/* We allready send off response once */
-	}
+	/* Repeat previous response in case of duplicate request */
+	if (!gtp_duplicate(gsn, version, peer, seq))
+		return 0;
 
 	/* Find the linked context in question */
 	if (pdp_getgtp1(&linked_pdp, get_tei(pack))) {
@@ -2638,7 +2655,7 @@ int gtp_gpdu_ind(struct gsn_t *gsn, int version,
 	return 0;
 }
 
-/* Receives GTP packet and sends off for further processing 
+/* Receives GTP packet and sends off for further processing.
  * Function will check the validity of the header. If the header
  * is not valid the packet is either dropped or a version not 
  * supported is returned to the peer. 

gtp/gtp.h

@@ -147,7 +147,7 @@ struct ul16_t;
 
 struct gtp0_header {		/*    Descriptions from 3GPP 09.60 */
 	uint8_t flags;		/* 01 bitfield, with typical values */
-	/*    000..... Version: 1 (0) */
+	/*    000..... Version: 0 */
 	/*    ...1111. Spare (7) */
 	/*    .......0 SNDCP N-PDU Number flag (0) */
 	uint8_t type;		/* 02 Message type. T-PDU = 0xff */
@@ -223,7 +223,7 @@ union gtp_packet {
  * application this struct is provided in order to store all
  * relevant information related to the gsn.
  * 
- * Note that this does not include information storage for '
+ * Note that this does not include information storage for
  * each pdp context. This is stored in another struct.
  *************************************************************/
 

gtp/gtpie.c

@@ -135,13 +135,14 @@ int gtpie_gettlv(union gtpie_member *ie[], int type, int instance,
 {
 	int ien;
 	ien = gtpie_getie(ie, type, instance);
-	if (ien >= 0) {
-		*length = ntoh16(ie[ien]->tlv.l);
-		if (*length <= size)
-			memcpy(dst, ie[ien]->tlv.v, *length);
-		else
-			return EOF;
-	}
+	if (ien < 0)
+		return EOF;
+
+	*length = ntoh16(ie[ien]->tlv.l);
+	if (*length > size)
+		return EOF;
+
+	memcpy(dst, ie[ien]->tlv.v, *length);
 	return 0;
 }
 
@@ -150,10 +151,9 @@ int gtpie_gettv0(union gtpie_member *ie[], int type, int instance,
 {
 	int ien;
 	ien = gtpie_getie(ie, type, instance);
-	if (ien >= 0)
-		memcpy(dst, ie[ien]->tv0.v, size);
-	else
+	if (ien < 0)
 		return EOF;
+	memcpy(dst, ie[ien]->tv0.v, size);
 	return 0;
 }
 

sgsnemu/sgsnemu.c

@@ -1267,7 +1267,8 @@ int cb_tun_ind(struct tun_t *tun, void *pack, unsigned len)
 	src.s_addr = iph->src;
 
 	if (ipget(&ipm, &src)) {
-		printf("Received packet without a valid source address!!!\n");
+		printf("Dropping packet from invalid source address: %s\n",
+		       inet_ntoa(src));
 		return 0;
 	}
 
@@ -1569,7 +1570,7 @@ int main(int argc, char **argv)
 						   512 = Flat rate, 256 = Hot billing */
 
 		/* Create context */
-		/* We send this of once. Retransmissions are handled by gtplib */
+		/* We send this off once. Retransmissions are handled by gtplib */
 		gtp_create_context_req(gsn, pdp, &iparr[n]);
 	}
 
@@ -1577,9 +1578,9 @@ int main(int argc, char **argv)
 
 	printf("Waiting for response from ggsn........\n\n");
 
-  /******************************************************************/
+	/******************************************************************/
 	/* Main select loop                                               */
-  /******************************************************************/
+	/******************************************************************/
 
 	while ((0 != state) && (5 != state)) {
 
@@ -1613,7 +1614,7 @@ int main(int argc, char **argv)
 				state = 3;
 		}
 
-		/* Send off disconnect */
+		/* Send disconnect */
 		if (3 == state) {
 			state = 4;
 			stoptime = time(NULL) + 5;	/* Extra seconds to allow disconnect */
@@ -1628,11 +1629,11 @@ int main(int argc, char **argv)
 			}
 		}
 
-		/* Send of ping packets */
+		/* Send ping packets */
 		diff = 0;
 		while ((diff <= 0) &&
 		       /* Send off an ICMP ping packet */
-		       /*if ( */ (options.pinghost.s_addr) && (2 == state) &&
+		       /*if */ (options.pinghost.s_addr) && (2 == state) &&
 		       ((pingseq < options.pingcount)
 			|| (options.pingcount == 0))) {
 			if (!pingseq)