Re: [PATCH gwproxy v8 2/2] gwproxy: refactor code base to add experimental raw DNS backend

[Ahmad Gani <reyuki@gnuweeb.org>]

On Fri, Sep 5, 2025 at 11:26 PM Alviro Iskandar Setiawan wrote:
> Having #ifdef CONFIG_RAW_DNS all over the place is horrible. Please do
> something else to reduce them. CTRL + F, "def CONFIG_RAW_DNS", 24
> matches.

Hi Sir Al, First I want to say thank you for reviewing my patchset (and
sir Ammar for pinging it).

I thought it was necessary to mark that a certain block belongs to the
experimental raw DNS feature. If I reduce the use of these marks, it might
introduce unnecessary code into the compiled version when this experimental
feature is disabled.

> On Fri, Aug 29, 2025 at 2:56 PM Ahmad Gani wrote:
>> +void cp_nsaddr(struct gwp_dns_ctx *ctx, struct gwp_sockaddr *addr, uint8_t *addrlen)
>> +{
>> + *addr = ctx->ns_addr;
>> + *addrlen = ctx->ns_addrlen;
>> +}
>
> Why is this exposed globally? I don't think it's worth a function
> call. Better be inlined.

I can't access members of the gwp_dns_ctx struct directly from another
file, as it seems to be treated as an opaque pointer outside of dns.c. If
I make it an inline function, it would only be accessible within dns.c.
Should I move the full declaration of the struct to the header file?

>> +int gwp_dns_process(struct gwp_dns_ctx *ctx, struct gwp_dns_entry *e)
>> +{
>> + struct gwdns_addrinfo_node *ai;
>> + uint8_t buff[UDP_MSG_LIMIT];
>> + ssize_t r;
>> +
>> + r = __sys_recvfrom(
>> + e->udp_fd, buff, sizeof(buff), 0,
>> + &ctx->ns_addr.sa, (socklen_t *)&ctx->ns_addrlen
>> + );
>> + if (r <= 0)
>> + return (int)r;
>
> The subsequent code calls gwdns_parse_query(), and you noted that a
> return value of 0 indicates success, and by the end of
> gwp_dns_process(), it returns what was returned by
> gwdns_parse_query(). That's it. It means, if successful, it returns 0.
> What happens if __sys_recvfrom() returns 0? Doesn't that mean it's
> also considered successful? If so, why?

Yeah, that's an error on my part. I will set r to -EINVAL for lengths in
the range 0-38. Since there is no short send in UDP, there's no way it
can be a valid standard query if the length is less than 38 bytes:
- 12 bytes for the header section
- 10 bytes for the query section
- 16 bytes for the answer section

The shortest valid domain length is 3 characters: 1 for the second-level
label and 2 for the TLD label. You can try `dig +short +noedns g.cn` and
observe it in Wireshark. So far, the shortest standard query response
I've found is 38 bytes.

>> + if (cfg->use_raw_dns) {
>> +#ifdef CONFIG_RAW_DNS
>> + r = convert_str_to_ssaddr(cfg->ns_addr_str, &ctx->ns_addr, 53);
>> + if (r)
>> goto out_destroy_mutex;
>> + ctx->ns_addrlen = ctx->ns_addr.sa.sa_family == AF_INET
>> + ? sizeof(ctx->ns_addr.i4)
>> + : sizeof(ctx->ns_addr.i6);
>> + ctx->entry_cap = DEFAULT_ENTRIES_CAP;
>> + ctx->entries = malloc(ctx->entry_cap * sizeof(*ctx->entries));
>> + if (!ctx->entries)
>> + goto out_destroy_mutex;
>> +#endif
>> + } else {
>
> Bad indentation. Apart from bad indentation, if the malloc() call
> fails, r still contains 0, and you goto out_destroy_mutex, returning 0
> on failure. Shouldn't you set -ENOMEM instead?

You're right to point that out, I'll fix it.

>> + txid = (uint16_t)rand();
>
> Why do you use a random number for the txid? I don't think rand()
> guarantees uniqueness. Also, I don't find the srand() call anywhere.
> That's equivalent to having srand(1).
>
> What about using a simple incrementing counter instead (which
> guarantees the uniqueness)?
> It will indeed reset after 65535, but that should give enough room to
> complete prior DNS requests first. With a simple counter, the reuse of
> an ID will always be 65535 queries away.

Without calling srand, it still generates random numbers, so I thought it
was fine. But I agree with your suggestion—an incrementing counter is
better, because randomness doesn’t guarantee uniqueness and is more
likely to cause collisions than a counter.

>> struct gwp_dns_entry {
>> +#ifdef CONFIG_RAW_DNS
>> + uint32_t idx;
>> + int udp_fd;
>
> AFAIU, when using the raw DNS feature you bring. ev_fd is not used,
> instead you use udp_fd. So I think it is not worth adding extra unused
> space. Better be an anonymous union if the name needs to be different.
> No?

That's indeed a more efficient way to handle it. Let's go with that idea.

>> +#ifdef CONFIG_RAW_DNS
>> +static int arm_poll_for_raw_dns_query(struct gwp_wrk *w,
>> + struct gwp_conn_pair *gcp)
>> +{
>> + struct gwp_dns_entry *gde = gcp->gde;
>> + struct gwp_dns_ctx *dctx;
>> + struct gwp_sockaddr addr;
>> + uint8_t addrlen;
>> + ssize_t r;
>> +
>> + dctx = w->ctx->dns;
>> + cp_nsaddr(dctx, &addr, &addrlen);
>> + r = __sys_sendto(
>> + gde->udp_fd, gde->payload, gde->payloadlen, MSG_NOSIGNAL,
>> + &addr.sa, addrlen
>> + );
>> +
>> + return (int)r;
>> +}
>> +#endif
>
> Why is this function named arm_poll_for_raw_dns_query()? There is no
> poll-arming here. You're simply calling __sys_sendto(). Please write a
> function name based on what it does. It's confusing.

Lol, I'm surprised myself to see that function name. I'm not sure what I
was thinking when I wrote it—sorry for the confusion.

> Bad indentation. Why are there many of those?

Sorry about the indentation issues. They often happen when I add a new if
block and forget to adjust the indentation afterward, I'll fix it.

--
Ahmad Gani