AdguardTeam / AdguardTeam/AdGuardHome

DHCP: a DISCOVER without a REQUEST persists a lease with a zero Expiry, which is never cleaned up

Aperta
#8,572 2 commenti 0 reazioni 1 assegnatario Assegnata a @EugeneOne1 Vedi su GitHub
Priority: P4 Resolution: Done Status: Ready for Deployment waiting for data
Lingua principale
TypeScript
Stelle
36.9k
Fork
2.5k
Metriche di merge delle PR
Nessuna PR unita negli ultimi 30g

Descrizione

### Prerequisites

- [X] I have checked the Wiki and Discussions and found no answer
- [X] I have searched other issues and found no duplicates
- [X] I want to report a bug and not ask a question
- [X] I have set up AdGuard Home correctly and configured clients to use it

### Platform (OS and CPU architecture)

Linux, AMD64 (aka x86_64)

### Installation

Custom (see the description below)

### Setup

On one machine

### AdGuard Home version

v0.107.78

### Action

Send a single `DHCPDISCOVER` to the AdGuard Home DHCP server and never follow it
with a `DHCPREQUEST` — i.e. exactly what a client does when it walks away
mid-negotiation (roaming between APs, MAC rotation, device going to sleep).

Minimal reproduction, using a made-up MAC so it is easy to spot afterwards:

```python
import socket, struct, random
xid = random.randbytes(4)
mac = bytes.fromhex('020000abcdef')
p = b'\x01\x01\x06\x00' + xid + b'\x00\x00\x80\x00'
p += b'\x00'*16 + mac + b'\x00'*10 + b'\x00'*192
p += b'\x63\x82\x53\x63' + b'\x35\x01\x01' + b'\x37\x03\x01\x03\x06' + b'\xff'
s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
s.setsockopt(socket.SOL_SOCKET, socket.SO_BROADCAST, 1)
s.bind(('0.0.0.0', 68))
s.sendto(p, ('255.255.255.255', 67)) # DISCOVER only — no REQUEST
```

Then look at `data/leases.json`.

### Expected result

A `DHCPDISCOVER` alone should not create a persistent lease. The address may be
reserved in memory while the offer is pending, but nothing should be written to
`leases.json` until the client confirms with a `DHCPREQUEST`.

### Actual result

A lease is written to `leases.json` immediately, with a **zero `Expiry`**:

```json
{"expires": "0001-01-01T00:00", "ip": "192.168.0.12",
"hostname": "", "mac": "02:00:00:ab:cd:ef", "static": false}
```

Because the expiry is the Go zero value rather than a real timestamp, the entry
is never treated as expired and never cleaned up. It stays in `leases.json`
indefinitely.

Looking at `internal/dhcpd/v4_unix.go`, this seems to be why:

```go
func (s *v4Server) handleDiscover(req, resp *dhcpv4.DHCPv4) (l *dhcpsvc.Lease, err error) {
mac := req.ClientHWAddr

defer s.conf.notify(LeaseChangedDBStore) // persists the lease DB
...
l, err = s.allocateLease(mac) // allocates, does not set Expiry
```

while `Expiry` is only ever set when the client actually confirms:

```go
func (s *v4Server) commitLease(l *dhcpsvc.Lease, hostname string) {
l.Expiry = time.Now().Add(s.conf.leaseTime)
```

So `handleDiscover` both allocates the lease *and* triggers a write of the lease
database, before the client has accepted anything.

### Additional information and/or screenshots

**Why this matters in practice.** On a network with a small DHCP pool these
entries accumulate until the pool is exhausted. On my setup the range is
`192.168.0.10`–`192.168.0.99` (90 addresses) with roughly 20 real devices. Every
phone that roams between two access points, and every iOS/Android private-MAC
rotation, leaves another zero-expiry lease behind.

Twice within 24 hours the pool reached **2 free addresses out of 90**. The
symptoms were not obvious:

- clients could no longer complete DHCP, and retried continuously
(~15 requests/second sustained)
- each request caused a rewrite of the whole lease file (~9 writes/second)
- **DNS resolution on the same process stopped answering entirely** — including
on `127.0.0.1` — because the server was busy serializing leases

So the visible failure was "DNS is down", several layers away from the actual
cause. Cleaning `leases.json` by hand and restarting resolved it both times.

**There is currently no supported way to clean this up.** Static leases can be
removed through the UI/API, but dynamic ones cannot be removed individually, and
"Reset all leases" also deletes static ones. This has been requested for years:

- #2164 — *Option to delete dynamic DHCP leases* (2020)
- #6804 — *Separate "Reset leases" button for non-static leases* (2024)
- #7901 — *Reset All Lease deletes static leases* (2025)

I suspect several of those requests are downstream of this same behaviour:
people accumulate leases they never really handed out, and ask for a button to
clear them. Users with a `/24`-sized pool would likely never notice, since the
junk would take years to fill the range.

**Possible fixes**, in what I would guess is increasing order of effort:

1. Do not persist the lease database from `handleDiscover` — only write on
commit.
2. Give allocated-but-unconfirmed leases a short expiry (e.g. the offer timeout)
so they age out on their own instead of living forever.
3. Skip entries whose `Expiry` is the zero value when loading `leases.json`, so
existing installations self-heal on restart.

Happy to test a patch or provide more data from a live instance.

Guida per i contributori

Apri la guida per i contributori

Valutazione

Questa issue non è ancora stata valutata.

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.