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.