[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #5955: irq_dispatch: fix array overrun in coverity check

[GitBox <gi...@apache.org>]

pkarashchenko commented on code in PR #5955:
URL: https://github.com/apache/incubator-nuttx/pull/5955#discussion_r842634154


##########
sched/irq/irq_dispatch.c:
##########
@@ -176,7 +176,13 @@ void irq_dispatch(int irq, FAR void *context)
 
   /* Then dispatch to the interrupt handler */
 
-  CALL_VECTOR(ndx, vector, irq, context, arg);

Review Comment:
   I think this fix is bad because is depends on `#if NR_IRQS > 0` and we have
   ```
   #ifdef CONFIG_ARCH_MINIMAL_VECTORTABLE
   struct irq_info_s g_irqvector[CONFIG_ARCH_NUSER_INTERRUPTS];
   #else
   struct irq_info_s g_irqvector[NR_IRQS];
   #endif
   ```
   and at least 3 implementations of `CAN_VECTOR`
   ```
   #ifndef CONFIG_SCHED_IRQMONITOR
   #  define CALL_VECTOR(ndx, vector, irq, context, arg) \
        vector(irq, context, arg)
   #elif defined(CONFIG_SCHED_CRITMONITOR)
   #  define CALL_VECTOR(ndx, vector, irq, context, arg) \
        do \
          { \
            struct timespec delta; \
            uint32_t start; \
            uint32_t elapsed; \
            start = up_perf_gettime(); \
            vector(irq, context, arg); \
            elapsed = up_perf_gettime() - start; \
            up_perf_convert(elapsed, &delta); \
            if (delta.tv_nsec > g_irqvector[ndx].time) \
              { \
                g_irqvector[ndx].time = delta.tv_nsec; \
              } \
            if (CONFIG_SCHED_CRITMONITOR_MAXTIME_IRQ > 0 && \
                elapsed > CONFIG_SCHED_CRITMONITOR_MAXTIME_IRQ) \
              { \
                serr("IRQ %d(%p), execute time too long %"PRIu32"\n", \
                     irq, vector, elapsed); \
              } \
          } \
        while (0)
   #else
   #  define CALL_VECTOR(ndx, vector, irq, context, arg) \
        do \
          { \
            struct timespec start; \
            struct timespec end; \
            struct timespec delta; \
            clock_systime_timespec(&start); \
            vector(irq, context, arg); \
            clock_systime_timespec(&end); \
            clock_timespec_subtract(&end, &start, &delta); \
            if (delta.tv_nsec > g_irqvector[ndx].time) \
              { \
                g_irqvector[ndx].time = delta.tv_nsec; \
              } \
          } \
        while (0)
   #endif /* CONFIG_SCHED_IRQMONITOR */
   ```
   I suspect that coverity generate issue only if `#if NR_IRQS > 0` but with proposed fix the `irq_unexpected_isr` will not be called at all if `NR_IRQS == 0`.
   Again I recommend to embed the check inside the proper variant of `CALL_VECTOR` macro



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org