On 22/05/2024 11:05 am, Jan Beulich wrote: > On 22.05.2024 11:56, scan-ad...@coverity.com wrote: >> ** CID 1598431: Memory - corruptions (OVERRUN) >> >> >> ________________________________________________________________________________________________________ >> *** CID 1598431: Memory - corruptions (OVERRUN) >> /xen/common/trace.c: 798 in trace() >> 792 } >> 793 >> 794 if ( rec_size > bytes_to_wrap ) >> 795 insert_wrap_record(buf, rec_size); >> 796 >> 797 /* Write the original record */ >>>>> CID 1598431: Memory - corruptions (OVERRUN) >>>>> Overrunning callee's array of size 28 by passing argument "extra" >>>>> (which evaluates to 31) in call to "__insert_record". >> 798 __insert_record(buf, event, extra, cycles, rec_size, extra_data); >> 799 >> 800 unlock: >> 801 spin_unlock_irqrestore(&this_cpu(t_lock), flags); >> 802 >> 803 /* Notify trace buffer consumer that we've crossed the high >> water mark. */ > How does the tool conclude "extra" evaluating to 31, when at the top of > the function it is clearly checked to be less than 28?
Which "top" ? The reasoning is: 2. Condition extra % 4UL /* sizeof (uint32_t) */, taking false branch. 3. Condition extra / 4UL /* sizeof (uint32_t) */ > 7, taking false branch. 4. cond_at_most: Checking extra / 4UL > 7UL implies that extra may be up to 31 on the false branch. which is where 31 comes from. What Coverity hasn't done is equated "<31 && multiple of 4" to mean "<28". I don't think this is unreasonable; analysis has to prune the reasoning somewhere... This is (fundamentally) a dumb-ABI problem where we're passing a byte count but only ever wanting to use it as a unit-of-uint32_t's count. But it's also problem that we're passing both extra and rec_size into __insert_record() when one is calculated from the other. I had decided to leave this alone for now, but maybe it could do with some improvements (simplifications) to the code. ~Andrew
