fix several latent bugs across parsing, OOM handling, and option validation#234
fix several latent bugs across parsing, OOM handling, and option validation#234vixie wants to merge 2 commits into
Conversation
…dation asinfo.c: inet_pton() returns 0 (not -1) on malformed addresses; the old check let bogus rdata through with an uninitialized stack buffer. Also use the canonical h_errno for the existence probe and propagate res_ninit() failure instead of charging ahead. dnsdbq.c: -O case in qparam_option() referred to optarg instead of its own arg parameter; harmless today but a footgun. wordexp() return is now checked before dereferencing we_wordv. Reject -p minimal -J at parse time -- ruminate_json() builds a query without a mode, which previously tripped abort() inside present_minimal_lookup(). netio.c: realloc() / asprintf() failure no longer slides into a NULL deref. The earlier asprintf cast turned a -1 return into SIZE_MAX and the realloc results were unchecked. Also handle fdopen() failure on the sort pipes. pdns.c: tuple_make() now requires rrname/rrtype/rdata up front, instead of leaving NULL pointers for sortable_rdatum() and the sort-key fprintf() to dereference on malformed records. time.c: avoid imaxabs(INTMAX_MIN) UB in the relative-time path. tokstr.h: drop a duplicate prototype. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cmikk
left a comment
cmikk
left a comment
There was a problem hiding this comment.
Requesting a few mostly style / consistency changes, plus clarification that the MAX_FETCH_BUF limit is sufficient for a properly operating supported pDNS API server, rather than a tunable (like MAX_FETCHES could be).
| writer_t writer = query->writer; | ||
| qparam_ct qp = &query->qp; | ||
| size_t bytes = size * nmemb; | ||
| char *x; |
There was a problem hiding this comment.
This variable should be factored out. Where it is used, a more straightforward:
final->buf = realloc(....);
if (final->buf == NULL)
my_panic(true, "realloc");
would be clearer.
| assert(writer->ps_buf == NULL && writer->ps_len == 0); | ||
| writer->ps_len = (size_t) | ||
| asprintf(&writer->ps_buf, "-- %s (%s)\n", | ||
| int x = asprintf(&writer->ps_buf, "-- %s (%s)\n", |
There was a problem hiding this comment.
Since we're retaining the return value of asprintf here in writer->ps_len, I would suggest giving it a different name from x, which is used elsewhere in this code base for a "throwaway after check" return value. In asinfo.c, for example, int n = asprintf(...) is used for this purpose.
|
|
||
| fetch->buf = realloc(fetch->buf, fetch->len + bytes); | ||
| if (fetch->len + bytes > MAX_FETCH_BUF) { | ||
| printf("?? very large response\n"); |
There was a problem hiding this comment.
Failure of this check should:
- use
my_logf()to print a warning rather thanprintf()ing to the output, and: - return
CURL_WRITEFUNC_ERROR, since returning0to signal error is not robust.
| * must not be greater than any pDNS system's concurrent connection limit. | ||
| */ | ||
| #define MAX_FETCHES 8 | ||
| #define MAX_FETCH_BUF (16*1024*1024) |
There was a problem hiding this comment.
It's not immediately clear from context whether this is a tunable resource limit or a "sanity check" guard against possible malformed output from the API server.
I believe it's the latter (and sufficient), based on the limited number of RRs which can fit in a 64KiB DNS message. This might be worth noting in a comment so this isn't confused for a tunable later.
4 participants
(from claude via zed.)
asinfo.c: inet_pton() returns 0 (not -1) on malformed addresses; the old check let bogus rdata through with an uninitialized stack buffer. Also use the canonical h_errno for the existence probe and propagate res_ninit() failure instead of charging ahead.
dnsdbq.c: -O case in qparam_option() referred to optarg instead of its own arg parameter; harmless today but a footgun. wordexp() return is now checked before dereferencing we_wordv. Reject -p minimal -J at parse time -- ruminate_json() builds a query without a mode, which previously tripped abort() inside present_minimal_lookup().
netio.c: realloc() / asprintf() failure no longer slides into a NULL deref. The earlier asprintf cast turned a -1 return into SIZE_MAX and the realloc results were unchecked. Also handle fdopen() failure on the sort pipes.
pdns.c: tuple_make() now requires rrname/rrtype/rdata up front, instead of leaving NULL pointers for sortable_rdatum() and the sort-key fprintf() to dereference on malformed records.
time.c: avoid imaxabs(INTMAX_MIN) UB in the relative-time path.
tokstr.h: drop a duplicate prototype.