Large improvement pack #192

baranyaib90 · 2025-08-24T19:55:59Z

Hi Aaron,
I hope you are doing well! I suppose you won't like me now. I have a lot of things for review.
Content list:

TCP support to fix TCP support #186
UDP reply truncation feature to fix https_dns_proxy + dnsmasq + systemd-resolved dnssec failure #184
Code quality stuff: more compiler warnings and warning fixes
c-ares API modernization
Reset HTTPS connection to avoid half-open TCP problems to fix A persistent timeout occurs when the network changes (static IP changed or PPPoE reconnected). #189
systemd improvements (mostly for my Ubuntu server)
Version bump (I guess it is time)

@stangri it I may ask, please remove file https://github.com/openwrt/packages/blob/master/net/https-dns-proxy/patches/010-cmakelists-remove-cflags.patch if you would uplift the OpenWRT package after this pull request would be merged. I have compiled the OpenWRT package myself and I have not met any warnings. If there would be, please let me know. Thanks!

Take your time.
Best regards,
Balázs

Preparations for TCP serving

- added TCP client limit option, also can be used to disable TCP DNS server in case of issues - readme and help pages updated - combined API with UDP DNS server - using minimally necessary public header - added TCP only statistic counters - added TCP functional tests - basic querries with valgrind - fragmented TCP request processing - testing 4k large response support - started to use stricter compiler warnings

To ensure code quality and make life harder. Notes: - choosed to use enum or int (e.g. ares_status_t) - CURLINFO_SSL_VERIFYRESULT was wrong - https_fetch_ctx_cleanup's curl_result_code parameter - int options range checked in options.c

If there are not any successful request.

Goal is to indicate service readyness after bootstrap has finished.

Because of larger feature pack

baranyaib90 · 2025-08-24T20:04:34Z

Ps.: I was testing my changes for 2 months and I have even asked issue reporters to test with proxy built from my repo if it works well. You can check the conversation on bug reports. I suppose it will be good enough.

aarond10 · 2025-08-25T04:05:57Z

bugbot run

cursor

Bugbot free trial expires on September 2, 2025
Learn more in the Cursor dashboard.

cursor · 2025-08-25T04:11:58Z

src/main.c

  https_client_init(&https_client, &opt, (opt.stats_interval ? &stat : NULL), loop);

+  struct addrinfo *listen_addrinfo = get_listen_address(opt.listen_addr);
+  ((struct sockaddr_in*) listen_addrinfo->ai_addr)->sin_port = htons((uint16_t)opt.listen_port);


Bug: DNS Server Fails IPv6 Port Handling

The DNS server incorrectly casts network address structures to sockaddr_in when accessing port numbers. This assumption breaks for IPv6 addresses, leading to memory corruption when setting ports and incorrect port values when extracting them, impacting both UDP and TCP operations.

Additional Locations (2)

src/dns_server.c#L27-L28

src/dns_server_tcp.c#L250-L251

cursor · 2025-08-25T04:11:58Z

src/dns_server_tcp.c

+  if (len <= 0) {
+    if (len == 0 || errno == ECONNRESET) {
+      DLOG_CLIENT("Connection closed");
+    } else if (errno == EAGAIN && errno == EWOULDBLOCK) {


Bug: Buffer Type Mismatch and Error Handling Bug

In read_cb, the recv call uses &buf instead of buf for the buffer, which is a type mismatch. Also, the errno check for EAGAIN and EWOULDBLOCK uses && instead of ||. This condition will never be true, causing non-blocking errors to be mishandled.

baranyaib90 · 2025-08-25T08:25:33Z

I have fixed the bugbot findings. Before merge lets squash the fixups.
The sin_port tricking was intentional, since the port is on the same memory area both in sockaddr_in and sockaddr_in6 structs. But lets be safe then.

aarond10 · 2025-08-25T11:58:51Z

src/dns_server.c

-  /* recvfrom can write to addrlen */
-  socklen_t tmp_addrlen = d->addrlen;
-  ssize_t len = recvfrom(w->fd, buf, REQUEST_MAX, 0, (struct sockaddr*)&raddr,
+  char tmp_buf[UINT16_MAX];  // stack buffer for largest UDP packet to support EDNS


I worry a little about this. It's probably OK but might blow up on some small systems with limited stack space.
We copy it into a heap allocated buffer anyway, so what about

char *dns_req = (char *)malloc(UINT16_MAX); recvfrom(...); ... dns_req = (char *)realloc(dns_req, len); if (dns_req == NULL) { ... }

Even on openwrt the stack limit is 8MB per thread. Stack size grows only by usage. For my taste 64k is not a huge deal and I would not mess with heap for temporary buffers. Also I would not reserve more on heap (and shrik later) to avoid heap fragmentation. So I'm not expecting any issues with this.
Anyway: I will think about this.

aarond10 · 2025-08-25T12:01:06Z

src/dns_server.c

+  return udp_size;
+}
+
+static void truncate_dns_response(char *buf, size_t *buflen, const uint16_t size_limit) {


wow what? :D What a long overkill function? well yes :(
I have tested this a lot manually with valgrind+dig not to make any mistakes, but still I'm not sure if it is 100% OK, but I hope that calling into this will happen extremely rarely, since EDNS0 UDP buffer limit is enough for most oversized response.

aarond10

These are some solid changes. Thanks! The UINT16_MAX stack allocation is the only thing that stands out as possibly problematic. Hope I didn't miss anything. :D

baranyaib90 · 2025-08-25T13:04:52Z

You are welcome! Lets not hurry, check again a few days later. I know, that it is a large change to take in...

baranyaib90 · 2025-08-28T20:09:25Z

I have been thinking about the stack buffer and I would like to keep it. My compromise is to lower the size from 64k to 4k. Before the limit was 1500 byte, so this should be more than enough. Now it will be logged if it was not enough. In case of TCP this does not cause issue, just the read callback will be called multiple times. I hope that this is acceptable by you, if not, then please let me know your proposal.

aarond10 · 2025-08-29T04:43:51Z

That sounds fine to me. I haven't used realloc much in the past and I like the idea that we are in control of the semantics of buffer management rather than using some niche libc call.

…

On Fri, 29 Aug 2025 at 06:09, baranyaib90 ***@***.***> wrote: *baranyaib90* left a comment (aarond10/https_dns_proxy#192) <#192 (comment)> I have been thinking about the stack buffer and I would like to keep it. My compromise is to lower the size from 64k to 4k. Before the limit was 1500 byte, so this should be more than enough. Now it will be logged if it was not enough. In case of TCP this does not cause issue, just the read callback will be called multiple times. I hope that this is acceptable by you, if not, then please let me know your proposal. — Reply to this email directly, view it on GitHub <#192 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABTOXWWF3CGIFF2ESXGFN33P5OYVAVCNFSM6AAAAACEV7UCE2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTEMZUG44TOMBWGE> . You are receiving this because you commented.Message ID: ***@***.***>

baranyaib90 · 2025-09-01T18:35:32Z

@stangri do you plan to make the new command line arguments configurable trough the luci page?

stangri · 2025-09-01T18:48:16Z

@stangri do you plan to make the new command line arguments configurable trough the luci page?

I've noticed the following changes, please let me know if I missed any:

  -T tcp_client_limit    Number of TCP clients to serve. (Default: 20, Disabled: 0, Min: 1, Max: 200)
  -q                     Use HTTP/3 (QUIC) only.
  -m max_idle_time       Maximum idle time in seconds allowed for reusing a HTTPS connection.
                         (Default: 118, Min: 0, Max: 3600)
  -L conn_loss_time      Time in seconds to tolerate connection timeouts of reused connections.
                         This option mitigates half-open TCP connection issue (e.g. WAN IP change).
                         (Default: 15, Min: 5, Max: 60)
  -s statistic_interval  Optional statistic printout interval.
                         (Default: 0, Disabled: 0, Min: 1, Max: 3600)
  -F log_limit           Flight recorder: storing desired amount of logs from all levels
                         in memory and dumping them on fatal error or on SIGUSR2 signal.
                         (Default: 0, Disabled: 0, Min: 100, Max: 100000)

I'll add support for uci configuration for these in the init script first. Like with some other packages I think the less used (or more expert-like) settings should be separated into the advanced tab. Once the init script/uci support these options it would be a trivial update (beyond adding tabs) to support these in the luci app too.

PS. So with the binary you can set these options per-instance. The WebUI for the instance settings already looks overloaded, so If you're willing to provide your feedback/confirm which settings have more to do with environment/OS and can be globalized (like the user/group should most likely be the same for all instances) and which settings might be more resolver-specific I'd be all ears.

baranyaib90 · 2025-09-01T19:26:32Z

Advanced tab sounds great!

Resolver specific:

HTTP/3 (QUIC) option if supported by openwrt libcurl
max_idle_time, since it depends on resolver server configuration

Other options are more like global.

stangri · 2025-09-01T23:39:50Z

Advanced tab sounds great!

The more I thought about integrating more of the CLI options into the init script, the more I think following makes sense:

the options can be defined either at the main config level or instance level
if the instance level option isn't defined, it's inherited from the main config level
the default value options are no longer added to the PROCD command

So for example:

config main 'config'
	option dnsmasq_config_update '*'
	option force_dns '1'
	list force_dns_port '53'

config https-dns-proxy
	option bootstrap_dns '1.1.1.1,1.0.0.1'
	option resolver_url 'https://cloudflare-dns.com/dns-query'
	option listen_addr '127.0.0.1'
	option listen_port '5053'
	option user 'nobody'
	option group 'nogroup'

config https-dns-proxy
	option bootstrap_dns '8.8.8.8,8.8.4.4'
	option resolver_url 'https://dns.google/dns-query'
	option listen_addr '127.0.0.1'
	option listen_port '5054'
	option user 'nobody'
	option group 'nogroup'

Can become more compact:

config main 'config'
	option dnsmasq_config_update '*'
	option force_dns '1'
	list force_dns_port '53'
	option listen_addr '127.0.0.1'
	option user 'nobody'
	option group 'nogroup'

config https-dns-proxy
	option bootstrap_dns '1.1.1.1,1.0.0.1'
	option resolver_url 'https://cloudflare-dns.com/dns-query'
	option listen_port '5053'

config https-dns-proxy
	option bootstrap_dns '8.8.8.8,8.8.4.4'
	option resolver_url 'https://dns.google/dns-query'
	option listen_port '5054'

And the PROCD command no longer includes -a 127.0.0.1 as it's a default value.

That gives flexibility in configuring and in adding options support to the WebUI.

That also means that I would have to update the init script in multiple places if some default values change upstream/here.

I've documented it at https://docs.openwrt.melmac.ca/https-dns-proxy/#global-settings-for-instances and CI is rebuilding 2025.09.01-r1 with the init file fixes.

baranyaib90 · 2025-09-02T06:37:10Z

Nice page. The only thing I'm missing is the settings value range. e.g. (Default: 0, Disabled: 0, Min: 1, Max: 3600)
Obviously it is up to you to include them or not.

stangri · 2025-09-02T19:16:18Z

Nice page. The only thing I'm missing is the settings value range. e.g. (Default: 0, Disabled: 0, Min: 1, Max: 3600) Obviously it is up to you to include them or not.

We're on the same page. I wanted to add another column with the value options, but the table is pretty narrow to fit the theme as it is. I might have to play with the theme/settings at some point to allow another column. All the docs are in this repo: https://github.com/stangri/docs.openwrt.melmac.ca

stangri · 2025-09-10T16:38:31Z

@baranyaib90 on 24.10.2 (x86-64) I got the following in dmesg:

[2379127.133000] traps: https-dns-proxy[18318] trap divide error ip:404bde sp:7ffeae0f68d0 error:0 in https-dns-proxy[403000+7000]
[2380210.516055] traps: https-dns-proxy[19980] trap divide error ip:404bde sp:7fff96e5f0d0 error:0 in https-dns-proxy[403000+7000]
[2394799.142337] traps: https-dns-proxy[22169] trap divide error ip:404bde sp:7ffcb278bcd0 error:0 in https-dns-proxy[403000+7000]

baranyaib90 · 2025-09-10T16:45:19Z

It looks like a divide by 0 error, but the location is unclear to me. If you can reproduce the issue, could you run the proxy with debug logs? I would need the last ~1000 logs of the stopped proxy.
Please open an issue for this.

stangri · 2025-09-10T16:55:45Z

Sorry, clarification: I was looking for something else at dmesg and saw these, I don't know how/when it happened, I'll try to reproduce but not sure if i can.

baranyaib90 · 2025-09-10T20:08:18Z

Actually you may not need to help me out with the reproduction. I have reviewed the new code I introduced, and I may have found the issue: answers could be 0 at 866a916#diff-593ced5c0d2c1e50378c33c17cd07cac9c6ba0856e3919474081ef40c68155abR142
I will try to come up with a fix Tomorrow.

baranyaib90 · 2025-09-11T20:05:49Z

I would like to ask you to build and test from my repo (https://github.com/baranyaib90/https_dns_proxy) to see if the division by 0 issue got fixed or not. Thanks!

stangri · 2025-09-12T00:02:36Z

Thanks @baranyaib90 I've built a binary from your repo. Any insight on how to try to trigger an event which previously resulted in a division by zero?

baranyaib90 · 2025-09-12T05:40:00Z

Well hard to tell actually. I suspect div by 0 could have happened in 2 ways: DNS response had:

No answer, just large authority and additional section, or
Way too many answers, like a few hundred.

I would recommend instead to:

Check dmesg with human date and try to recall what have you done. E.g. with browser history check what sites were you visiting and visit those sites again.
Lets wait 1 week and check if crash have happened or not.

baranyaib90 · 2025-09-19T06:21:05Z

Hi @stangri, are there any new crashes mentioned in dmesg since?
I'm planning to raise the pull request soon.

stangri · 2025-09-20T23:15:48Z

Hey @baranyaib90, nothing in dmesg. Your change either worked or at least did no harm, so I'd vote in favour of PR/merging this code. I'll create PR for the OpenWrt package once your code is merged in this repo.

baranyaib90 added 8 commits June 15, 2025 23:35

DNS server refactor

49c6aed

Preparations for TCP serving

UDP truncation feature

866a916

Enable more compiler warnings

083d8d4

To ensure code quality and make life harder. Notes: - choosed to use enum or int (e.g. ares_status_t) - CURLINFO_SSL_VERIFYRESULT was wrong - https_fetch_ctx_cleanup's curl_result_code parameter - int options range checked in options.c

Replacing c-ares deprecated functions

7c5445c

Reset HTTPS client on timeouts

58bad2e

If there are not any successful request.

systemd service improvements

7626a98

Goal is to indicate service readyness after bootstrap has finished.

Version bump

f10d561

Because of larger feature pack

cursor bot reviewed Aug 25, 2025

View reviewed changes

baranyaib90 added 2 commits August 25, 2025 10:21

fixup! DNS server refactor

887635f

fixup! Added DNS server with TCP support

acdcaf7

baranyaib90 force-pushed the master branch from 4a5965f to acdcaf7 Compare August 25, 2025 08:23

aarond10 reviewed Aug 25, 2025

View reviewed changes

aarond10 requested changes Aug 25, 2025

View reviewed changes

baranyaib90 added 2 commits August 28, 2025 22:01

fixup! DNS server refactor

dab40d4

fixup! Added DNS server with TCP support

3f35626

aarond10 merged commit daa0d39 into aarond10:master Sep 1, 2025
2 checks passed

baranyaib90 mentioned this pull request Sep 21, 2025

Fixes 16 #193

Merged

Large improvement pack #192

Large improvement pack #192

Uh oh!

Conversation

baranyaib90 commented Aug 24, 2025

Uh oh!

baranyaib90 commented Aug 24, 2025

Uh oh!

aarond10 commented Aug 25, 2025

Uh oh!

cursor bot left a comment

Choose a reason for hiding this comment

Uh oh!

cursor bot Aug 25, 2025

Choose a reason for hiding this comment

Bug: DNS Server Fails IPv6 Port Handling

Uh oh!

cursor bot Aug 25, 2025

Choose a reason for hiding this comment

Bug: Buffer Type Mismatch and Error Handling Bug

Uh oh!

baranyaib90 commented Aug 25, 2025

Uh oh!

aarond10 Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

baranyaib90 Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

aarond10 Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

baranyaib90 Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

aarond10 left a comment

Choose a reason for hiding this comment

Uh oh!

baranyaib90 commented Aug 25, 2025

Uh oh!

baranyaib90 commented Aug 28, 2025

Uh oh!

aarond10 commented Aug 29, 2025 via email

Uh oh!

Uh oh!

baranyaib90 commented Sep 1, 2025

Uh oh!

stangri commented Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

baranyaib90 commented Sep 1, 2025

Uh oh!

stangri commented Sep 1, 2025

Uh oh!

baranyaib90 commented Sep 2, 2025

Uh oh!

stangri commented Sep 2, 2025

Uh oh!

stangri commented Sep 10, 2025

Uh oh!

baranyaib90 commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stangri commented Sep 10, 2025

Uh oh!

baranyaib90 commented Sep 10, 2025

Uh oh!

baranyaib90 commented Sep 11, 2025

Uh oh!

stangri commented Sep 12, 2025

Uh oh!

baranyaib90 commented Sep 12, 2025

Uh oh!

baranyaib90 commented Sep 19, 2025

Uh oh!

stangri commented Sep 20, 2025

Uh oh!

Reviewers

Assignees

Labels

stangri commented Sep 1, 2025 •

edited

Loading

baranyaib90 commented Sep 10, 2025 •

edited

Loading