----- Original Message -----
> On 06/03/2013 10:20 AM, Andrew Jones wrote:
> > Coverity complains about two overruns in process_tx_desc(). The
> > complaints are false positives, but we might as well eliminate
> > them. The problem is that "hdr" is defined as an unsigned int,
> > but then used to offset an array of size 65536, and another of
> > size 256 bytes. hdr will actually never be greater than 255
> > though, as it's assigned only once and to the value of
> > tp->hdr_len, which is an uint8_t. This patch simply gets rid of
> > hdr, replacing it with tp->hdr_len, which makes it consistent
> > with all other tp member use in the function.
> >
> > Signed-off-by: Andrew Jones <drjo...@redhat.com>
> > ---
> > hw/net/e1000.c | 16 ++++++++--------
> > 1 file changed, 8 insertions(+), 8 deletions(-)
> >
>
> The logic looks sound, but checkpatch detects some style issues. See below.
>
> > diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> > index e6f46f0c511e8..eec3e7a4524d1 100644
> > --- a/hw/net/e1000.c
> > +++ b/hw/net/e1000.c
> > @@ -556,7 +556,7 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc
> > *dp)
> > uint32_t txd_lower = le32_to_cpu(dp->lower.data);
> > uint32_t dtype = txd_lower & (E1000_TXD_CMD_DEXT | E1000_TXD_DTYP_D);
> > unsigned int split_size = txd_lower & 0xffff, bytes, sz, op;
> > - unsigned int msh = 0xfffff, hdr = 0;
> > + unsigned int msh = 0xfffff;
> > uint64_t addr;
> > struct e1000_context_desc *xp = (struct e1000_context_desc *)dp;
> > struct e1000_tx *tp = &s->tx;
> > @@ -603,8 +603,7 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc
> > *dp)
> >
> > addr = le64_to_cpu(dp->buffer_addr);
> > if (tp->tse && tp->cptse) {
> > - hdr = tp->hdr_len;
> > - msh = hdr + tp->mss;
> > + msh = tp->hdr_len + tp->mss;
> > do {
> > bytes = split_size;
> > if (tp->size + bytes > msh)
> > @@ -612,14 +611,15 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc
> > *dp)
> >
> > bytes = MIN(sizeof(tp->data) - tp->size, bytes);
> > pci_dma_read(&s->dev, addr, tp->data + tp->size, bytes);
> > - if ((sz = tp->size + bytes) >= hdr && tp->size < hdr)
> > - memmove(tp->header, tp->data, hdr);
> > + if ((sz = tp->size + bytes) >= tp->hdr_len
> > + && tp->size < tp->hdr_len)
> > + memmove(tp->header, tp->data, tp->hdr_len);
>
> The 'if' statement above needs some braces. Checkpatch also isn't wild about
> the assignment inside of the conditional.
>
> > tp->size = sz;
> > addr += bytes;
> > if (sz == msh) {
> > xmit_seg(s);
> > - memmove(tp->data, tp->header, hdr);
> > - tp->size = hdr;
> > + memmove(tp->data, tp->header, tp->hdr_len);
> > + tp->size = tp->hdr_len;
> > }
> > } while (split_size -= bytes);
> > } else if (!tp->tse && tp->cptse) {
> > @@ -633,7 +633,7 @@ process_tx_desc(E1000State *s, struct e1000_tx_desc
> > *dp)
> >
> > if (!(txd_lower & E1000_TXD_CMD_EOP))
> > return;
> > - if (!(tp->tse && tp->cptse && tp->size < hdr))
> > + if (!(tp->tse && tp->cptse && tp->size < tp->hdr_len))
> > xmit_seg(s);
>
> Braces here as well.
>
> > tp->tso_frames = 0;
> > tp->sum_needed = 0;
> >
>
> Although the style issues were present to begin with, we may as well take
> the opportunity to fix them.
Running checkpatch on the file yields
142 errors, 41 warnings
I could send a v2 that fixes the 1 error and 2 warnings found in the context
of this patch, but why? It's out of the scope of the patch (although I did
use "cleanup" in the summary...), and it would hardly make a dent in this
file's problems.
drew
>
> Sincerely,
>
> Jesse Larrew
> Software Engineer, KVM Team
> IBM Linux Technology Center
> Phone: (512) 973-2052 (T/L: 363-2052)
> jlar...@linux.vnet.ibm.com
>
>
