Re: [PATCH 06/13] resolv: Add DNS packet parsing helpers geared towards wire format

[Siddhesh Poyarekar <siddhesh@gotplt.org>]

On 2022-08-19 10:59, Florian Weimer wrote:
> * Siddhesh Poyarekar:
> 
>>> +bool
>>> +__ns_rr_cursor_next (struct ns_rr_cursor *c, struct ns_rr_wire *rr)
>>> +{
>>> +  rr->rdata = NULL;
>>> +
>>> +  /* Extract the record owner name.  */
>>> +  int consumed = __ns_name_unpack (c->begin, c->end, c->current,
>>> +                                   rr->rname, sizeof (rr->rname));
>>> +  if (consumed < 0)
>>> +    {
>>> +      memset (rr, 0, sizeof (*rr));
>>> +      __set_errno (EMSGSIZE);
>>> +      return false;
>>> +    }
>>> +  c->current += consumed;
>>> +
>>> +  /* Extract the metadata.  */
>>> +  struct
>>> +  {
>>> +    uint16_t rtype;
>>> +    uint16_t rclass;
>>> +    uint32_t ttl;
>>> +    uint16_t rdlength;
>>> +  } __attribute__ ((packed)) metadata;
>>> +  _Static_assert (sizeof (metadata) == 10, "sizeof metadata");
>>> +  if (c->end - c->current < sizeof (metadata))
>>> +    {
>>> +      memset (rr, 0, sizeof (*rr));
>>> +      __set_errno (EMSGSIZE);
>>> +      return false;
>>> +    }
>>> +  memcpy (&metadata, c->current, sizeof (metadata));
>>
>> Doesn't this go out of sync with the init above?  The initialization
>> appears to put current just after rclass (with current += 4).
> 
> Do you mean __ns_rr_cursor_init?  The question section has a different
> entry layout than the other sections.
> 
> Quoting RFC 1035:
> 
> | 4.1.2. Question section format
> |
> | The question section is used to carry the "question" in most queries,
> | i.e., the parameters that define what is being asked.  The section
> | contains QDCOUNT (usually 1) entries, each of the following format:
> |
> |                                     1  1  1  1  1  1
> |       0  1  2  3  4  5  6  7  8  9  0  1  2  3  4  5
> |     +--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+
> |     |                                               |
> |     /                     QNAME                     /
> |     /                                               /
> |     +--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+
> |     |                     QTYPE                     |
> |     +--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+
> |     |                     QCLASS                    |
> |     +--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+
> 
> Versus:
> 
> | 4.1.3. Resource record format
> |
> | The answer, authority, and additional sections all share the same
> | format: a variable number of resource records, where the number of
> | records is specified in the corresponding count field in the header.
> | Each resource record has the following format:
> |                                     1  1  1  1  1  1
> |       0  1  2  3  4  5  6  7  8  9  0  1  2  3  4  5
> |     +--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+
> |     |                                               |
> |     /                                               /
> |     /                      NAME                     /
> |     |                                               |
> |     +--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+
> |     |                      TYPE                     |
> |     +--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+
> |     |                     CLASS                     |
> |     +--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+
> |     |                      TTL                      |
> |     |                                               |
> |     +--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+
> |     |                   RDLENGTH                    |
> |     +--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--|
> |     /                     RDATA                     /
> |     /                                               /
> |     +--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+
> 
> At least I hope this is what your question is about. 8-)

Indeed I mixed up the two.  Let me resume review from there then.

Thanks,
Sid