net/arp/arp_table.c: fix ARP table validity time calculation overflow issue

[nuttxs]

Summary

net/arp/arp_table.c: prevent overflow in macro expansion when setting a long ARP timeout.
Currently, when tickless mode is enabled, the value of CONFIG_USEC_PER_TICK=100. During the macro expansion of SEC2TICK(), the calculated value exceeds UINT32_MAX, leading to an overflow problem.

Impact

New Feature/Change: Issue fix (no new feature).
User Impact: Prevent file descriptor leakage.
Build Impact:No new Kconfig options or build system changes.
Hardware Impact: No
Security: No
Compatibility: Backward-compatible; no breaking changes.

Testing

Overflow issue occurred:
SEC2TICK(108640)=5006541
Correctly convert results:
SEC2TICK(108640)=864000000

[hartmannathan]

This should fix the problem when CONFIG_SYSTEM_TIME64. However please note that when not CONFIG_SYSTEM_TIME64, clock_t can still be 32-bit:

See:

nuttx/include/sys/types.h

Line 247 in 1df6aa9

/* Used for system times in clock ticks. This type is the natural width of

/* Used for system times in clock ticks. This type is the natural width of
 * the system timer.
 *
 * NOTE: The signed-ness of clock_t is not specified at OpenGroup.org.  An
 * unsigned type is used to support the full range of the internal clock.
 */

#ifdef CONFIG_SYSTEM_TIME64
typedef uint64_t     clock_t;
typedef uint64_t     time_t;         /* Holds time in seconds */
#else
typedef uint32_t     clock_t;
typedef uint32_t     time_t;         /* Holds time in seconds */
#endif
typedef int          clockid_t;      /* Identifies one time base source */
typedef FAR void    *timer_t;        /* Represents one POSIX timer */

I am not sure what we can do about that.

[nuttxs]

This should fix the problem when CONFIG_SYSTEM_TIME64. However please note that when not CONFIG_SYSTEM_TIME64, clock_t can still be 32-bit:

See:

nuttx/include/sys/types.h

Line 247 in 1df6aa9

/* Used for system times in clock ticks. This type is the natural width of

/* Used for system times in clock ticks. This type is the natural width of
 * the system timer.
 *
 * NOTE: The signed-ness of clock_t is not specified at OpenGroup.org.  An
 * unsigned type is used to support the full range of the internal clock.
 */

#ifdef CONFIG_SYSTEM_TIME64
typedef uint64_t     clock_t;
typedef uint64_t     time_t;         /* Holds time in seconds */
#else
typedef uint32_t     clock_t;
typedef uint32_t     time_t;         /* Holds time in seconds */
#endif
typedef int          clockid_t;      /* Identifies one time base source */
typedef FAR void    *timer_t;        /* Represents one POSIX timer */

I am not sure what we can do about that.

When CONFIG_SYSTEM_TIME64 is enabled.
Even if clock_t is 64 bits, passing a 32-bit parameter can cause overflow during multiplication:

• SEC2TICK(86400) treats 86400 as a 32-bit int.
• This leads to overflow in the multiplication 86400 * 1000000 when converting to microseconds

[xiaoxiang781216]

#14460 suggest to drop 32bit support.

[nuttxs]

#14460 suggest to drop 32bit support.
This is a feasible solution. However, most embedded processors today are 32-bit. Completely drop 32-bit(time_t) support maybe incur unnecessary computation and storage overhead.

[raiden00pl]

@nuttxs NuttX follows POSIX, and POSIX-2024 explicitly states that time_t must be 64-bit.

Dropping time_t as 32-bit is the only option and will most likely happen in release 13.0.0

[nuttxs]

@raiden00pl Thanks for sharing the info on the time_t update, which is related to fixing the Y2038 issue
The current ticket involves an overflow issue in macro expansion.
Will upgrading time_t involve the macro definitions in clock.h?

[hartmannathan]

When CONFIG_SYSTEM_TIME64 is enabled. Even if clock_t is 64 bits, passing a 32-bit parameter can cause overflow during multiplication:

* `SEC2TICK(86400)` treats 86400 as a 32-bit `int`.

* This leads to overflow in the multiplication `86400 * 1000000` when converting to microseconds

Must ARP really use microseconds? Can a lower resolution be used, like seconds?

For example:

-#define ARP_MAXAGE_TICK SEC2TICK(10 * CONFIG_NET_ARP_MAXAGE)
+#define ARP_MAXAGE_SEC  (10 * CONFIG_NET_ARP_MAXAGE)

 /****************************************************************************
  * Name: arp_lookup
  *
  * Description:
  *   Find the ARP entry corresponding to this IP address in the ARP table.
  *
  * Input Parameters:
  *   ipaddr - Refers to an IP address in network order
  *   dev    - Device structure
  *
  * Assumptions:
  *   The network is locked to assure exclusive access to the ARP table.
  *   The return value will become unstable when the network is unlocked.
  *
  ****************************************************************************/
 
 static FAR struct arp_entry_s *arp_lookup(in_addr_t ipaddr,
                                           FAR struct net_driver_s *dev)
 {
   FAR struct arp_entry_s *tabptr;
   int i;
 
   /* Check if the IPv4 address is already in the ARP table. */
 
   for (i = 0; i < CONFIG_NET_ARPTAB_SIZE; ++i)
     {
       tabptr = &g_arptable[i];
       if (tabptr->at_dev == dev &&
           net_ipv4addr_cmp(ipaddr, tabptr->at_ipaddr) &&
-          clock_systime_ticks() - tabptr->at_time <= ARP_MAXAGE_TICK)
+          TICK2SEC(clock_systime_ticks()) - TICK2SEC(tabptr->at_time) <= ARP_MAXAGE_SEC)
         {
           return tabptr;
         }
     }
 
   /* Not found */
 
   return NULL;
 }

Thoughts?

[nuttxs]

@hartmannathan
Accurate to the second should be a solution to avoid the current overflow issue.
Here are some thoughts on the "type casting" and "comparing in seconds" approaches:
Approach 1: Only a one - line code change is needed. It addresses the issue at compile time, incurs no runtime calculation overhead, and doesn't impact other code.
Approach 2: Adds runtime overhead due to necessary division operations during each comparison. Multiple code call sites need to be modified.
If option 2 is more suitable, I will also modify the current submission.
Please correct any inadequacies in the above analysis. Thank you!

[nuttxs]

@hartmannathan Are there any other comments on the current ticket? Or are there better suggestions?