Re: [PATCH gwproxy v5 2/2] dnsparser: Add dns parser code

[Alviro Iskandar Setiawan <alviro.iskandar@gnuweeb.org>]

On Thu, Aug 14, 2025 at 11:53 AM Ahmad Gani wrote:
> +static int serialize_answ(uint16_t txid, uint8_t *in, size_t in_len, gwdns_answ_data *out)
> +{
> +       gwdns_header_pkt *hdr;
> +       uint16_t raw_flags;
> +       size_t idx, i;
> +       void *ptr;
> +       int ret;
> +
> +       idx = sizeof(*hdr);
> +       if (idx >= in_len)
> +               return -EAGAIN;
> +
> +       hdr = (void *)in;
> +       if (memcmp(&txid, &hdr->id, sizeof(txid)))
> +               return -EINVAL;
> +
> +       memcpy(&raw_flags, &hdr->flags, sizeof(raw_flags));
> +       raw_flags = ntohs(raw_flags);
> +       /* QR MUST 1 = response from dns server */
> +       if (!DNS_QR(raw_flags))
> +               return -EINVAL;
> +
> +       /* OPCODE MUST 0 = standard query */
> +       if (DNS_OPCODE(raw_flags))
> +               return -EINVAL;
> +
> +       /* RCODE MUST 0 = No error */
> +       if (DNS_RCODE(raw_flags))
> +               return -EPROTO;
> +
> +       // is it safe or recommended to alter the in buffer directly?
> +       hdr->ancount = ntohs(hdr->ancount);
> +       if (!hdr->ancount)
> +               return -ENODATA;
> +
> +       /*
> +        * Check the sizes upfront.
> +        *
> +        * 1 bytes for variable-length
> +        * in[idx] for the length of first name
> +        * 1 bytes for null terminator
> +        * 2 bytes for qtype
> +        * 2 bytes for qclass
> +        */
> +       if ((size_t)(1 + in[idx] + 1 + 2 + 2) >= in_len)
> +               return -EINVAL;
> +
> +       ret = calculate_question_len(&in[idx], in_len - idx);
> +       if (ret <= 0)
> +               return -EINVAL;
> +
> +       idx += ret;
> +       if (idx >= in_len)
> +               return -EAGAIN;
> +
> +       out->hdr.ancount = 0;
> +       ptr = malloc(hdr->ancount * sizeof(uint8_t *));
> +       if (!ptr)
> +               return -ENOMEM;
> +
> +       out->rr_answ = ptr;
> +       for (i = 0; i < hdr->ancount; i++) {
> +               uint16_t is_compressed, rdlength;
> +               gwdns_serialized_answ *item = malloc(sizeof(gwdns_serialized_answ));
> +               if (!item) {
> +                       ret = -ENOMEM;
> +                       goto exit_free;
> +               }
> +
> +               memcpy(&is_compressed, &in[idx], sizeof(is_compressed));
> +               is_compressed = DNS_IS_COMPRESSED(ntohs(is_compressed));
> +               assert(is_compressed);
> +               idx += 2; // NAME
> +               if (idx >= in_len) {
> +                       ret = -EAGAIN;
> +                       free(item);
> +                       goto exit_free;
> +               }
> +
> +               memcpy(&item->rr_type, &in[idx], 2);
> +               item->rr_type = ntohs(item->rr_type);
> +               idx += 2; // TYPE
> +               if (idx >= in_len) {
> +                       ret = -EAGAIN;
> +                       free(item);
> +                       goto exit_free;
> +               }
> +               memcpy(&item->rr_class, &in[idx], 2);
> +               item->rr_class = ntohs(item->rr_class);
> +               idx += 2; // CLASS
> +               if (idx >= in_len) {
> +                       ret = -EAGAIN;
> +                       free(item);
> +                       goto exit_free;
> +               }
> +               memcpy(&item->ttl, &in[idx], 4);
> +               item->ttl = be32toh(item->ttl);
> +               idx += 4; // TTL
> +               if (idx >= in_len) {
> +                       ret = -EAGAIN;
> +                       free(item);
> +                       goto exit_free;
> +               }
> +
> +               memcpy(&rdlength, &in[idx], sizeof(rdlength));
> +               rdlength = ntohs(rdlength);
> +               if (item->rr_type != TYPE_AAAA && item->rr_type != TYPE_A) {
> +                       ret = -EINVAL;
> +                       free(item);
> +                       goto exit_free;
> +               }
> +               if (item->rr_type == TYPE_AAAA && rdlength != sizeof(struct in6_addr)) {
> +                       ret = -EINVAL;
> +                       free(item);
> +                       goto exit_free;
> +               }
> +               if (item->rr_type == TYPE_A && rdlength != sizeof(struct in_addr)) {
> +                       ret = -EINVAL;
> +                       free(item);
> +                       goto exit_free;
> +               }
> +               item->rdlength = rdlength;
> +               idx += 2;
> +               if (idx >= in_len) {
> +                       ret = -EAGAIN;
> +                       free(item);
> +                       goto exit_free;
> +               }
> +
> +               /*
> +                * considering if condition above,
> +                * maybe we don't need a malloc and just allocate fixed size
> +                * for rdata? however if this parser want to be expanded for
> +                * other dns operation (e.g OPCODE_IQUERY, etc), rdata maybe
> +                * contain more than sizeof in6_addr.
> +                */
> +               ptr = malloc(rdlength);
> +               if (!ptr) {
> +                       ret = -ENOMEM;
> +                       free(item);
> +                       goto exit_free;
> +               }
> +
> +               memcpy(ptr, &in[idx], rdlength);
> +               idx += rdlength;
> +               if (idx > in_len) {
> +                       ret = -EINVAL;
> +                       free(item);
> +                       free(ptr);
> +                       goto exit_free;
> +               }
> +
> +               item->rdata = ptr;
> +               out->rr_answ[i] = item;
> +               out->hdr.ancount++;
> +       }
> +
> +       return 0;
> +exit_free:
> +       for (i = 0; i < out->hdr.ancount; i++) {
> +               free(out->rr_answ[i]->rdata);
> +               free(out->rr_answ[i]);
> +       }
> +       free(out->rr_answ);
> +       return ret;
> +}

I think free(item); can be deduplicated. It's repeated several times
here. Consider using goto exit_free_item and add the appropriate
label. Do the same to other places if you have the same pattern
elsewhere.