enable clean up of NAT STUN state, prevents test_plugin_udp from segfaulting

2 files changed, 101 insertions, 64 deletions

diff --git a/src/nat/nat.c b/src/nat/nat.c
index 3573ec497..b0ec4fe19 100644
--- a/src/nat/nat.c
+++ b/src/nat/nat.c
@@ -270,17 +270,17 @@ struct GNUNET_NAT_Handle
   /**
    * ID of select gnunet-helper-nat-server stdout read task
    */
-  struct GNUNET_SCHEDULER_Task * server_read_task;
+  struct GNUNET_SCHEDULER_Task *server_read_task;
 
   /**
    * ID of interface IP-scan task
    */
-  struct GNUNET_SCHEDULER_Task * ifc_task;
+  struct GNUNET_SCHEDULER_Task *ifc_task;
 
   /**
    * ID of hostname DNS lookup task
    */
-  struct GNUNET_SCHEDULER_Task * hostname_task;
+  struct GNUNET_SCHEDULER_Task *hostname_task;
 
   /**
    * ID of DynDNS lookup task
@@ -288,6 +288,11 @@ struct GNUNET_NAT_Handle
   struct GNUNET_SCHEDULER_Task *dns_task;
 
   /**
+   * Active STUN request, if any.
+   */
+  struct GNUNET_NAT_STUN_Handle *stun_request;
+
+  /**
    * How often do we scan for changes in our IP address from our local
    * interfaces?
    */
@@ -1109,12 +1114,11 @@ list_interfaces (void *cls,
 }
 
 
-
 /**
- * Callback if the STun request have a error
+ * Callback with the result from the STUN request.
  *
  * @param cls the NAT handle
- * @param result , the status
+ * @param result the status
  */
 static void
 stun_request_callback (void *cls,
@@ -1122,14 +1126,15 @@ stun_request_callback (void *cls,
 {
   struct GNUNET_NAT_Handle *h = cls;
 
-  if (NULL == h)
-    return;
-  h->waiting_stun = GNUNET_NO;
-
-  if(result != GNUNET_OK)
+  h->stun_request = NULL;
+  if (GNUNET_NAT_ERROR_SUCCESS != result)
   {
     LOG (GNUNET_ERROR_TYPE_WARNING,
-       "Error processing a STUN request");
+         "Error processing a STUN request");
+  }
+  else
+  {
+    h->waiting_stun = GNUNET_YES;
   }
 }
 
@@ -1153,7 +1158,7 @@ GNUNET_NAT_is_valid_stun_packet (void *cls,
   struct sockaddr_in answer;
 
   /* We are not expecting a STUN message */
-  if (! h->waiting_stun)
+  if (GNUNET_YES != h->waiting_stun)
     return GNUNET_NO;
 
   /* We dont have STUN installed */
@@ -1166,14 +1171,14 @@ GNUNET_NAT_is_valid_stun_packet (void *cls,
           sizeof(struct sockaddr_in));
 
   /*Lets handle the packet*/
-  int valid = GNUNET_NAT_stun_handle_packet (data,
-                                             len,
-                                             &answer);
-  if (! valid)
+  if (GNUNET_NO ==
+      GNUNET_NAT_stun_handle_packet (data,
+                                     len,
+                                     &answer))
     return GNUNET_NO;
 
   LOG (GNUNET_ERROR_TYPE_INFO,
-       "Stun server returned %s:%d\n",
+       "STUN server returned %s:%d\n",
        inet_ntoa (answer.sin_addr),
        ntohs (answer.sin_port));
   /* Remove old IPs from previous STUN calls */
@@ -1208,16 +1213,19 @@ process_stun (void *cls,
        "I will request the stun server %s:%i\n",
        elem->address,
        elem->port);
-  if (GNUNET_OK ==
-      GNUNET_NAT_stun_make_request (elem->address,
+  if (NULL != h->stun_request)
+  {
+    GNUNET_NAT_stun_make_request_cancel (h->stun_request);
+    h->stun_request = NULL;
+  }
+  h->waiting_stun = GNUNET_NO;
+  h->stun_request
+    = GNUNET_NAT_stun_make_request (elem->address,
                                     elem->port,
                                     h->socket,
                                     &stun_request_callback,
-                                    NULL))
-  {
-    h->waiting_stun = GNUNET_YES;
-  }
-  else
+                                    h);
+  if (NULL == h->stun_request)
   {
     LOG (GNUNET_ERROR_TYPE_ERROR,
          "STUN request to %s:%i failed\n",
@@ -1226,7 +1234,8 @@ process_stun (void *cls,
   }
   h->stun_task =
     GNUNET_SCHEDULER_add_delayed (h->stun_frequency,
-                                  &process_stun, h);
+                                  &process_stun,
+                                  h);
 
   /* Set actual Server*/
   if (NULL != elem->next)
@@ -1709,7 +1718,6 @@ GNUNET_NAT_register (const struct GNUNET_CONFIGURATION_Handle *cfg,
       /* Set the actual STUN server*/
       h->actual_stun_server = h->stun_servers_head;
     }
-
     h->stun_task = GNUNET_SCHEDULER_add_now (&process_stun,
                                              h);
   }
@@ -1821,10 +1829,18 @@ GNUNET_NAT_unregister (struct GNUNET_NAT_Handle *h)
     GNUNET_SCHEDULER_cancel (h->stun_task);
     h->stun_task = NULL;
   }
+  if (NULL != h->stun_request)
+  {
+    GNUNET_NAT_stun_make_request_cancel (h->stun_request);
+    h->stun_request = NULL;
+  }
   if (NULL != h->server_proc)
   {
-    if (0 != GNUNET_OS_process_kill (h->server_proc, GNUNET_TERM_SIG))
-      GNUNET_log_from_strerror (GNUNET_ERROR_TYPE_WARNING, "nat", "kill");
+    if (0 != GNUNET_OS_process_kill (h->server_proc,
+                                     GNUNET_TERM_SIG))
+      GNUNET_log_from_strerror (GNUNET_ERROR_TYPE_WARNING,
+                                "nat",
+                                "kill");
     GNUNET_OS_process_wait (h->server_proc);
     GNUNET_OS_process_destroy (h->server_proc);
     h->server_proc = NULL;
diff --git a/src/nat/nat_stun.c b/src/nat/nat_stun.c
index 2ed78030f..e97b0016b 100644
--- a/src/nat/nat_stun.c
+++ b/src/nat/nat_stun.c
@@ -154,14 +154,20 @@ encode_message (enum StunClasses msg_class,
 static const char *
 stun_msg2str(int msg)
 {
-  static const struct { enum StunClasses value; const char *name; } classes[] = {
+  static const struct {
+    enum StunClasses value;
+    const char *name;
+  } classes[] = {
     { STUN_REQUEST, "Request" },
     { STUN_INDICATION, "Indication" },
     { STUN_RESPONSE, "Response" },
     { STUN_ERROR_RESPONSE, "Error Response" },
     { 0, NULL }
   };
-  static const struct { enum StunMethods value; const char *name; } methods[] = {
+  static const struct {
+    enum StunMethods value;
+    const char *name;
+  } methods[] = {
     { STUN_BINDING, "Binding" },
     { 0, NULL }
   };
@@ -201,9 +207,12 @@ stun_msg2str(int msg)
  * @return string with the attribute name
  */
 static const char *
-stun_attr2str(int msg)
+stun_attr2str (int msg)
 {
-  const struct { enum StunAttributes value; const char *name; } attrs[] = {
+  static const struct {
+    enum StunAttributes value;
+    const char *name;
+  } attrs[] = {
     { STUN_MAPPED_ADDRESS, "Mapped Address" },
     { STUN_RESPONSE_ADDRESS, "Response Address" },
     { STUN_CHANGE_ADDRESS, "Change Address" },
@@ -242,7 +251,7 @@ stun_attr2str(int msg)
  * @param req, stun header to be filled
  */
 static void
-generate_request_id(struct stun_header *req)
+generate_request_id (struct stun_header *req)
 {
   unsigned int x;
 
@@ -292,7 +301,8 @@ stun_get_mapped (struct StunState *st,
   default:
     return 1;
   }
-  if (ntohs(attr->len) < 8 && returned_addr->family != 1)
+  if ( (ntohs(attr->len) < 8) &&
+       (returned_addr->family != 1) )
   {
     return 1;
   }
@@ -371,10 +381,7 @@ GNUNET_NAT_stun_handle_packet (const void *data,
          (int)len);
     return GNUNET_NO;
   }
-  else
-  {
-    len = advertised_message_size;
-  }
+  len = advertised_message_size;
   memset (&st,0, sizeof(st));
 
   while (len > 0)
@@ -415,15 +422,21 @@ GNUNET_NAT_stun_handle_packet (const void *data,
 
 
 /**
- * Clean-up used memory
+ * Cancel active STUN request. Frees associated resources
+ * and ensures that the callback is no longer invoked.
  *
- * @param handle handle to release memory for
+ * @param rh request to cancel
  */
-static void
-clean (struct GNUNET_NAT_STUN_Handle *handle)
+void
+GNUNET_NAT_stun_make_request_cancel (struct GNUNET_NAT_STUN_Handle *rh)
 {
-  GNUNET_free (handle->stun_server);
-  GNUNET_free (handle);
+  if (NULL != rh->dns_active)
+  {
+    GNUNET_RESOLVER_request_cancel (rh->dns_active);
+    rh->dns_active = NULL;
+  }
+  GNUNET_free (rh->stun_server);
+  GNUNET_free (rh);
 }
 
 
@@ -439,32 +452,35 @@ stun_dns_callback (void *cls,
                    const struct sockaddr *addr,
                    socklen_t addrlen)
 {
-  struct GNUNET_NAT_STUN_Handle *request = cls;
+  struct GNUNET_NAT_STUN_Handle *rh = cls;
   struct stun_header *req;
   uint8_t reqdata[1024];
   int reqlen;
   struct sockaddr_in server;
 
+  rh->dns_active = NULL;
   if (NULL == addr)
   {
-    request->dns_active = NULL;
-    if (GNUNET_NO == request->dns_success)
+    if (GNUNET_NO == rh->dns_success)
     {
       LOG (GNUNET_ERROR_TYPE_INFO,
            "Error resolving host %s\n",
-           request->stun_server);
-      request->cb (request->cb_cls,
-                   GNUNET_NAT_ERROR_NOT_ONLINE);
-      clean (request);
+           rh->stun_server);
+      rh->cb (rh->cb_cls,
+              GNUNET_NAT_ERROR_NOT_ONLINE);
+      GNUNET_NAT_stun_make_request_cancel (rh);
     }
     return;
   }
 
-  request->dns_success= GNUNET_YES;
-  memset(&server,0, sizeof(server));
+  rh->dns_success = GNUNET_YES;
+  memset (&server,0, sizeof(server));
   server.sin_family = AF_INET;
   server.sin_addr = ((struct sockaddr_in *)addr)->sin_addr;
-  server.sin_port = htons(request->stun_port);
+  server.sin_port = htons(rh->stun_port);
+#if HAVE_SOCKADDR_IN_SIN_LEN
+  server.sin_len = (u_char) sizeof (struct sockaddr_in);
+#endif
 
   /*Craft the simplest possible STUN packet. A request binding*/
   req = (struct stun_header *)reqdata;
@@ -476,7 +492,7 @@ stun_dns_callback (void *cls,
   req->msgtype = htons(encode_message(STUN_REQUEST, STUN_BINDING));
 
   /* Send the packet */
-  if (-1 == GNUNET_NETWORK_socket_sendto (request->sock,
+  if (-1 == GNUNET_NETWORK_socket_sendto (rh->sock,
                                           req,
                                           ntohs(req->msglen) + sizeof(*req),
                                           (const struct sockaddr *) &server,
@@ -484,11 +500,14 @@ stun_dns_callback (void *cls,
   {
     GNUNET_log_strerror (GNUNET_ERROR_TYPE_ERROR,
                          "Fail to sendto");
-    request->cb (request->cb_cls,
-                 GNUNET_NAT_ERROR_INTERNAL_NETWORK_ERROR);
-    clean (request);
+    rh->cb (rh->cb_cls,
+            GNUNET_NAT_ERROR_INTERNAL_NETWORK_ERROR);
+    GNUNET_NAT_stun_make_request_cancel (rh);
     return;
   }
+  /* sending STUN request done, let's wait for replies... */
+  rh->cb (rh->cb_cls,
+          GNUNET_NAT_ERROR_SUCCESS);
 }
 
 
@@ -503,9 +522,9 @@ stun_dns_callback (void *cls,
  * @param sock the socket used to send the request
  * @param cb callback in case of error
  * @param cb_cls closure for @a cb
- * @return #GNUNET_OK success, #GNUNET_NO on error.
+ * @return NULL on error
  */
-int
+struct GNUNET_NAT_STUN_Handle *
 GNUNET_NAT_stun_make_request (const char *server,
                               uint16_t port,
                               struct GNUNET_NETWORK_Handle *sock,
@@ -529,8 +548,10 @@ GNUNET_NAT_stun_make_request (const char *server,
   {
     GNUNET_log_strerror (GNUNET_ERROR_TYPE_ERROR,
                          "Failed DNS");
-    clean (rh);
-    return GNUNET_NO;
+    GNUNET_NAT_stun_make_request_cancel (rh);
+    return NULL;
   }
-  return GNUNET_OK;
+  return rh;
 }
+
+/* end of nat_stun.c */