On Sun, 21 Jun 2020 at 13:54, Philippe Mathieu-Daudé <f4...@amsat.org> wrote: > > From: Yoshinori Sato <ys...@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