Hi Jukka,

2014-07-10 10:12 GMT+02:00 Jukka Rissanen <[email protected]>:

> Hi Mario,
>
> On ke, 2014-07-09 at 16:52 +0200, Mario Schuknecht wrote:
> > 4-byte memory access works only when pointer is 4-byte aligned. Use
> memcpy
> > function, since 4-byte pointer alignment is not guaranteed. The same
> applies
> > to 2-byte memory access.
>
> What hardware do you see this issue? At least the Intel and ARM devices
> I have used work just fine as is.
>
>
I see the problem on Marvell Kirkwood 88F6282 ARM SOC. The issue is
reproducible.
The pointer 'c' points to a 2-byte boundary. As a result the domain
question Class
field, that is before TTL field is set to zero and thus the cache contents
is invalid.

I would prefer the use << operator instead of memcpy here. See usage
> examples in dnsproxy.c for details.
>
> Should I change it like this?
                /* now the 4 byte TTL field */
                i = htonl(new_ttl);
                c[0] = (unsigned char)((i)     & 0xFF);
                c[1] = (unsigned char)((i>>8)  & 0xFF);
                c[2] = (unsigned char)((i>>16) & 0xFF);
                c[3] = (unsigned char)((i>>24) & 0xFF);
                c += 4;
                len -= 4;
                if (len < 0)
                        break;

                /* now the 2 byte rdlen field */
                w = c[0] | c[1] << 8;
                c += ntohs(w) + 2;
                len -= ntohs(w) + 2;

And

        /* now the query, which is a name and 2 16 bit words */
        l = dns_name_length(c) + 1;
        c += l;
        w = c[0] | c[1] << 8;
        type = ntohs(w);

Regards

Mario


> >
> > Signed-off-by: Mario Schuknecht <[email protected]>
> > ---
> >  src/dnsproxy.c | 20 ++++++++++----------
> >  1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/dnsproxy.c b/src/dnsproxy.c
> > index 3e32b5a..57f28fa 100644
> > --- a/src/dnsproxy.c
> > +++ b/src/dnsproxy.c
> > @@ -356,8 +356,8 @@ static int dns_name_length(unsigned char *buf)
> >  static void update_cached_ttl(unsigned char *buf, int len, int new_ttl)
> >  {
> >       unsigned char *c;
> > -     uint32_t *i;
> > -     uint16_t *w;
> > +     uint32_t i;
> > +     uint16_t w;
> >       int l;
> >
> >       /* skip the header */
> > @@ -387,17 +387,17 @@ static void update_cached_ttl(unsigned char *buf,
> int len, int new_ttl)
> >                       break;
> >
> >               /* now the 4 byte TTL field */
> > -             i = (uint32_t *)c;
> > -             *i = htonl(new_ttl);
> > +             i = htonl(new_ttl);
> > +             memcpy(c, &i, 4);
> >               c += 4;
> >               len -= 4;
> >               if (len < 0)
> >                       break;
> >
> >               /* now the 2 byte rdlen field */
> > -             w = (uint16_t *)c;
> > -             c += ntohs(*w) + 2;
> > -             len -= ntohs(*w) + 2;
> > +             memcpy(&w, c, 2);
> > +             c += ntohs(w) + 2;
> > +             len -= ntohs(w) + 2;
> >       }
> >  }
> >
> > @@ -1344,7 +1344,7 @@ static void cache_refresh(void)
> >  static int reply_query_type(unsigned char *msg, int len)
> >  {
> >       unsigned char *c;
> > -     uint16_t *w;
> > +     uint16_t w;
> >       int l;
> >       int type;
> >
> > @@ -1358,8 +1358,8 @@ static int reply_query_type(unsigned char *msg,
> int len)
> >       /* now the query, which is a name and 2 16 bit words */
> >       l = dns_name_length(c) + 1;
> >       c += l;
> > -     w = (uint16_t *) c;
> > -     type = ntohs(*w);
> > +     memcpy(&w, c, 2);
> > +     type = ntohs(w);
> >
> >       return type;
> >  }
>
>
> Cheers,
> Jukka
>
>
>
_______________________________________________
connman mailing list
[email protected]
https://lists.connman.net/mailman/listinfo/connman

Reply via email to