Pluggable per-entry params (delay/allow) - #12
Conversation
|
@droid review |
There was a problem hiding this comment.
Code Review - Critical Issues Found
I've identified several critical issues that need to be fixed:
1. Buffer overflow risk in delayed_push (rbldnsd.c:1323)
Issue: The function copies peer address without validating peerlen fits in sockaddr_storage.
Location: Lines 1323-1324 in rbldnsd.c
r->peerlen = peerlen;
memcpy(&r->peer, peer, peerlen);Fix: Add bounds check before memcpy:
if (peerlen > sizeof(r->peer)) {
free(copy);
return 0;
}
r->peerlen = peerlen;
memcpy(&r->peer, peer, peerlen);2. Integer overflow in time calculation (rbldnsd_entryparams.c:167)
Issue: Addition of ts + delay_sec can overflow when both values are large (e.g., ts near ULLONG_MAX).
Location: Line 167 in rbldnsd_entryparams.c
unsigned long long ready = ts + (unsigned long long)delay_sec;Fix: Check for overflow before addition:
if (ts > ULLONG_MAX - delay_sec) {
// Overflow would occur, treat as already ready or handle appropriately
return 0;
}
unsigned long long ready = ts + (unsigned long long)delay_sec;3. Sendto failure doesn't handle delayed_bytes correctly (rbldnsd.c:1296)
Issue: If sendto() fails (returns -1), delayed_bytes is still decremented and the buffer is freed. This can cause delayed_bytes underflow on repeated failures.
Location: Lines 1295-1297 in rbldnsd.c
if (r.buf && r.len) {
(void)sendto(r.fd, r.buf, r.len, 0, (struct sockaddr *)&r.peer, r.peerlen);
delayed_bytes -= r.len;
free(r.buf);
}Fix: Only decrement if send succeeds or handle failure:
if (r.buf && r.len) {
ssize_t sent = sendto(r.fd, r.buf, r.len, 0, (struct sockaddr *)&r.peer, r.peerlen);
if (sent >= 0 || errno == EINTR || errno == EAGAIN) {
delayed_bytes -= r.len;
}
free(r.buf);
}4. Missing allocation size overflow check (rbldnsd_entryparams.c:278)
Issue: The calculation sizeof(*params) + sizeof(params->kv[0]) * npairs can overflow if npairs is very large.
Location: Line 278 in rbldnsd_entryparams.c
struct kv_params *params =
mp_alloc(mp, sizeof(*params) + sizeof(params->kv[0]) * npairs, 1);Fix: Check for overflow before allocation:
size_t kv_size = sizeof(params->kv[0]) * npairs;
if (npairs > 0 && kv_size / npairs != sizeof(params->kv[0])) {
return NULL; // overflow
}
struct kv_params *params = mp_alloc(mp, sizeof(*params) + kv_size, 1);5. Heap size can exceed DELAYED_MAX_ITEMS (rbldnsd.c:1320)
Issue: After realloc succeeds, delayed_heap_len is incremented at line 1330 without re-checking against DELAYED_MAX_ITEMS.
Location: Lines 1320-1330 in rbldnsd.c
Fix: The check at line 1308 should be sufficient, but the doubling logic at 1316-1318 ensures new_cap never exceeds DELAYED_MAX_ITEMS, so this is actually safe. However, consider adding an assertion for safety.
These issues should be addressed to ensure memory safety and prevent potential DoS vectors.
|
Code review completed successfully. The review has been submitted. |
Summary
This PR adds pluggable per-entry key/value parameters to selected dataset types and implements built-in policies for delayed/cooldown entries and aclkey-aware gating.
Supported dataset types (entries accept params after
@):dnhaship4trieip6trieEntry params syntax
Params are parsed after
@and can be delimited by:or;:Implemented behaviors
1) Cooldown (suppressed until ready)
For entries with both
ts=<unix_timestamp>anddelay=<duration>:now >= ts + delay.delayuses the existingparse_time()semantics (e.g.60,60s,10m,1h, etc.).2) aclkey integration
These keys are interpreted in entry params and depend on whether the request is keyed (
NSQUERY_KEY, set by theaclkeydataset when a key label is present):key=requirekey=nodelayts+delay) is ignoreddelaywithoutts) is disabledImplementation notes
struct kv_params) and passed torbldnsd_apply_entry_params()during lookup.ts/delay/keysemantics.struct entry_actionnow hasflagsto allow a handler to stop further processing / indicate no-delay.Tests
Functional tests added/extended:
test/functional/cases/005_ENTRY_PARAMS.robotts+delaykey=requireblocks unkeyed but allows keyed querieskey=nodelaybypasses cooldown for keyed queriesValidation
test/pyunit/tests.py)test/functional/cases)