Hi Yoshinori, On 6/25/20 11:25 AM, Peter Maydell wrote: > 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.
Can you address Peter's comments please? > > thanks > -- PMM >