Bug Reports: RTCS NAT server in MQX 4.1.1

[chrissolomon]

Hi,

I have been working on getting the NAT server going on 4.1.1. I have had it going in 4.0.2, so I wasn't expecting any drama, but I have come across a few issues:

1)  In alggen.c the "#if RTCSCFG_ENABLE_NAT" is before rtcs.h is included (which includes user_config.h) so defining RTCSCFG_ENABLE_NAT in the user config doesn't cause this file to be compiled.

Fix:

@@ -26,5 +26,8 @@
*
*END************************************************************************/

+#include <rtcs.h>
+#include <rtcs_prv.h>
+
#if RTCSCFG_ENABLE_NAT

@@ -29,6 +32,4 @@
#if RTCSCFG_ENABLE_NAT

-#include <rtcs.h>
-#include <rtcs_prv.h>
#include <fio.h>
#include <tftp.h>

The same problem is present in natftp.c, Fix:

@@ -25,5 +25,8 @@
 *
 *
 *END************************************************************************/
+#include <rtcs.h>
+#include <rtcs_prv.h>
+
 #if RTCSCFG_ENABLE_NAT

@@ -28,7 +31,5 @@
 #if RTCSCFG_ENABLE_NAT

-#include <rtcs.h>
-#include <rtcs_prv.h>
 #include <fio.h>
 #include <tftp.h>
 #include <ctype.h>

and in nattftp.c, fix:

@@ -25,6 +25,8 @@
 *
 *
 *END************************************************************************/
+#include <rtcs.h>
+#include <rtcs_prv.h>

 #if RTCSCFG_ENABLE_NAT

@@ -28,5 +30,4 @@

 #if RTCSCFG_ENABLE_NAT

-#include <rtcs.h>
 #include <fio.h>
@@ -32,4 +33,3 @@
 #include <fio.h>
-#include <rtcs_prv.h>
 #include <tftp.h>

@@ -34,6 +34,5 @@
 #include <tftp.h>

-
 #include "nat.h"
 #include "nat_prv.h"

2) In nat_prv.h the function pointer typedefs were still using "pointer" type (which is no longer present in MQX 4.1.1)

Fix:

@@ -427,7 +427,7 @@

typedef void (_CODE_PTR_ NAT_ALG_FREE_FUNC)
(
-   pointer
+   void *
);

extern NAT_ALG_FREE_FUNC NAT_alg_free_func_table[];
@@ -444,7 +444,7 @@

extern uint32_t NAT_ALG_tftp
(
-   pointer
+   void *
);

extern uint32_t NAT_ALG_tftp_apply
@@ -462,7 +462,7 @@

extern uint32_t NAT_ALG_ftp
(
-   pointer
+   void *
);

3) In nat_apply.c I believe a bug has been introduced with the type changes.

Line 510 reads:

unsigned char               *embedded_checksum_ptr = NULL, port_ptr, replacement_ptr;

so, embedded_checksum_ptr is a pointer, but the other two are not.

Fix:

@@ -507,7 +507,9 @@
    NAT_CFG_STRUCT_PTR      nat_cfg_ptr = RTCS_getcfg(NAT);
    IP_HEADER_PTR           embedded_ip_header_ptr;
    TRANSPORT_UNION         transport, embedded_transport;
-   unsigned char               *embedded_checksum_ptr = NULL, port_ptr, replacement_ptr;
+   unsigned char               *embedded_checksum_ptr = NULL;
+   unsigned char               *port_ptr;
+   unsigned char               *replacement_ptr;
    _ip_address             new_ip, old_ip;
    bool                 icmpicmp = FALSE;
    uint16_t                 checksum, embedded_checksum, data_len, new_port, old_port;

4) Probably not causing an issue, but in dnat.c the source and destination port variables are not initialised, leading to a warning.

Fix:

@@ -57,7 +57,8 @@
    _ip_address                source_ip = mqx_ntohl(ip_header_ptr->SOURCE);
    _ip_address                target_ip = mqx_ntohl(ip_header_ptr->DEST);
    uint32_t                    ip_protocol;
-   uint16_t                    source_port, destination_port; /* source port and destination port */
+   uint16_t                    source_port = 0;
+   uint16_t   destination_port = 0; /* source port and destination port */

    transport.PTR = TRANSPORT_PTR(ip_header_ptr);

5) Similar niggle - in natdata.c I get a warning that key is not initialised - Fix was to initialise to zero.

6) And again, in natevent.c I get a warning that tmp_nat_event_ptr is not initialised. Fix was to initialise to null.

7) This one is a bit odd - there is a function call in sh_nat.c, in Shell_natinfo, to a function called status, but there isn't any function by that name.

I would guess, given the context, that we want to check the NAT server is up and running, but there doesn't seem to be an API to indicate this.

The closest I can come up with is to call NAT_networks - if there is no NAT config it returns null, so my fix was to add the following function:

static bool status() {
  return NAT_networks() != NULL;
}

Thanks

Chris

[chrissolomon]

And another: In natftp the type change has led to another pointer not being a pointer (like 3 above) -

@@ -278,7 +278,8 @@
    NAT_CFG_STRUCT_PTR         nat_cfg_ptr = RTCS_getcfg(NAT);
    NAT_SESSION_STRUCT_PTR     nat_session_ptr = *((NAT_SESSION_STRUCT_PTR *)session_ptr_ptr);
    NAT_SESSION_STRUCT_PTR     nat_data_session_ptr;
-   unsigned char                  *orig_pcb_data_ptr = (void *)RTCSPCB_DATA(*pcb_ptr_ptr), new_pcb_data_ptr;
+   unsigned char                  *orig_pcb_data_ptr = (void *)RTCSPCB_DATA(*pcb_ptr_ptr);
+   unsigned char  *new_pcb_data_ptr;
    IP_HEADER_PTR              ip_header_ptr = (IP_HEADER_PTR)(void *)orig_pcb_data_ptr;
    TRANSPORT_UNION            transport;
    NAT_SESSION_FTP_STRUCT_PTR ftp_session_ptr;

This is why I was always taught not to declare more than one variable per line, it can catch you out when pointers are involved.