Subject: Re: [PATCH] ares_parse_srv_reply

Re: [PATCH] ares_parse_srv_reply

From: Uli <ToBeSpammed_at_web.de>
Date: Thu, 02 Jul 2009 17:35:45 +0200

Jakub Hrozek wrote:
> Hello,
> thanks for reviewing the patch!
>
> On 07/01/2009 08:06 PM, Uli wrote:
>> Hi,
>
>> This looks like it will break badly if those fields aren't 2 byte large (they
>> aren't)
>
> My main source of documentation when writing the function was RFC 2782
> [1], which specifically says "This is a 16 bit unsigned integer in
> network byte order." in description of the port, weight and priority
> fields on pages 2 and 3 of the RFC. I also checked the source of ISC
> Bind 9.6.1, the structure for SRV records is declared in file
> lib/dns/rdata/in_1/srv_33.h and the fields are declared as isc_uint16_t
> there.
>
> Actually, I changed the weight, priority and port members of struct
> srv_reply to be u_int16_t as they were plain int in the previous patch.
> New version of the patch is attached.

Ok, this should fix my doubts. :)

> (Related question: would you prefer using u_int_XX_t from sys/types.h
> which is unconditionally included in ares.h or rather ISO C99-compliant
> uint_XX_t types from stdint.h?)

Don't ask me, I'm not a regular here.

>> and if the mem isn't initialized to zero (it isn't, allocated via malloc
>> which doesn't initialize mem).
>
> I don't quite understand why is this a problem. The uninitialized value
> of both bytes is overwritten with the memcpy call.

Yeah, but before you had only initialized the lower 2 bytes of a e.g. 4 byte
large integer which meant memcpy() didn't completely overwrite it.
>
>> Oh and I'd expect this to break badly on some endian where it writes to the high
>> bytes of the "int"s.
>
>
> I expected the bytes in the ares payload (aptr) to be in the network
> byte order and that's why I converted them to host byte order with
> ntohs. I'm not sure what's wrong with this. Can you elaborate, please?

With "this" I meant again the memcpy() call.

In case it wasn't clear (sorry!): My only complaint was that you used memcpy()
to initialize only part of a larger integer variable. If you use some *int16_t
variable this is fixed.

Uli

-- 
"Do you know that books smell like nutmeg or some spice from a foreign land?"
                                                  -- Faber in Fahrenheit 451
Received on 2009-07-02