Re: [PULL 07/15] hw/timer: RX62N 8-Bit timer (TMR)

[Peter Maydell]

On Sun, 21 Jun 2020 at 13:54, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> From: Yoshinori Sato <ysato@users.sourceforge.jp>
>
> renesas_tmr: 8bit timer modules.

Hi; the recent Coverity run reports a potential bug in this
code: (CID 1429976)


> +static uint16_t read_tcnt(RTMRState *tmr, unsigned size, int ch)
> +{
> +    int64_t delta, now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +    int elapsed, ovf = 0;
> +    uint16_t tcnt[2];

Here we declare tcnt[] but do not initialize its contents...

> +    uint32_t ret;
> +
> +    delta = (now - tmr->tick) * NANOSECONDS_PER_SECOND / tmr->input_freq;
> +    if (delta > 0) {
> +        tmr->tick = now;
> +
> +        if (FIELD_EX8(tmr->tccr[1], TCCR, CSS) == INTERNAL) {
> +            /* timer1 count update */
> +            elapsed = elapsed_time(tmr, 1, delta);
> +            if (elapsed >= 0x100) {
> +                ovf = elapsed >> 8;
> +            }
> +            tcnt[1] = tmr->tcnt[1] + (elapsed & 0xff);
> +        }
> +        switch (FIELD_EX8(tmr->tccr[0], TCCR, CSS)) {
> +        case INTERNAL:
> +            elapsed = elapsed_time(tmr, 0, delta);
> +            tcnt[0] = tmr->tcnt[0] + elapsed;
> +            break;
> +        case CASCADING:
> +            if (ovf > 0) {
> +                tcnt[0] = tmr->tcnt[0] + ovf;
> +            }
> +            break;
> +        }

...but not all cases here set both tcnt[0] and tcnt[1]
(for instance in the "case CASCADING:" if ovf <=0 we
won't set either of them)...

> +    } else {
> +        tcnt[0] = tmr->tcnt[0];
> +        tcnt[1] = tmr->tcnt[1];
> +    }
> +    if (size == 1) {
> +        return tcnt[ch];
> +    } else {
> +        ret = 0;
> +        ret = deposit32(ret, 0, 8, tcnt[1]);
> +        ret = deposit32(ret, 8, 8, tcnt[0]);
> +        return ret;

...and so here we will end up returning uninitialized
data. Presumably the spec says what value is actually
supposed to be returned in these cases?

PS: the "else" branch with the deposit32() calls could be
rewritten more simply as
  return lduw_be_p(tcnt);

> +static uint64_t tmr_read(void *opaque, hwaddr addr, unsigned size)
> +{

In this function Coverity reports a missing "break" (CID 1429977):

> +    case A_TCORA:
> +        if (size == 1) {
> +            return tmr->tcora[ch];
> +        } else if (ch == 0) {
> +            return concat_reg(tmr->tcora);
> +        }

Here execution can fall through but there is no 'break' or '/* fallthrough */'.

> +    case A_TCORB:
> +        if (size == 1) {
> +            return tmr->tcorb[ch];
> +        } else {
> +            return concat_reg(tmr->tcorb);
> +        }

Is it correct that the A_TCORA and A_TCORB code is different?
It looks odd, so if this is intentional then a comment describing
why it is so might be helpful to readers.

thanks
-- PMM