Skip to content

fix several latent bugs across parsing, OOM handling, and option validation #234

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vixie wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix several latent bugs across parsing, OOM handling, and option validation #234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a8699cb
fix several latent bugs across parsing, OOM handling, and option vali…
vixie May 9, 2026
70ca31c
more from claude code + opus 3.7
vixie May 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@ CWARN +=-Werror
CDEFS = -DWANT_PDNS_DNSDB=1 -DWANT_PDNS_CIRCL=1
CGPROF =
CDEBUG = -g -O3
CFLAGS += $(CGPROF) $(CDEBUG) $(CWARN) $(CDEFS)
CPROT = -fstack-protector-strong \
-D_FORTIFY_SOURCE=2 \
-fPIE -pie -Wl,-z,relro,-z,now
CFLAGS += $(CGPROF) $(CDEBUG) $(CWARN) $(CDEFS) $(CPROT)
INCL= $(CURLINCL) $(JANSINCL)
LIBS= $(CURLLIBS) $(JANSLIBS) -lresolv
# For freebsd, it requires that -lresolv _not_ be used here, use this instead of the above line:
Expand Down
7 changes: 4 additions & 3 deletions asinfo.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ asinfo_domain_exists(const char *domain) {
u_char buf[NS_PACKETSZ];

return res_query(domain, ns_c_in, ns_t_txt, buf, sizeof buf) > 0 ||
_res.res_h_errno != HOST_NOT_FOUND;
h_errno != HOST_NOT_FOUND;
}

/* asinfo_shutdown() -- deallocate underlying library's heap resources
Expand All @@ -108,7 +108,7 @@ asinfo_from_ipv4(const char *addr, char **asnum, char **cidr) {
u_char a4[32/8];
char *dname;

if (inet_pton(AF_INET, addr, a4) < 0)
if (inet_pton(AF_INET, addr, a4) == 0)
return strdup(strerror(errno));
int n = asprintf(&dname, "%d.%d.%d.%d.%s",
a4[3], a4[2], a4[1], a4[0], asinfo_domain);
Expand Down Expand Up @@ -175,7 +175,8 @@ asinfo_from_dns(const char *dname, char **asnum, char **cidr) {

DEBUG(1, true, "asinfo_from_dns(%s)\n", dname);
if ((res.options & RES_INIT) == 0)
res_ninit(&res);
if (res_ninit(&res) < 0)
return strdup("res_ninit failed");
n = res_nquery(&res, dname, ns_c_in, ns_t_txt, buf, sizeof buf);
if (n < 0) {
if (res.res_h_errno == HOST_NOT_FOUND)
Expand Down
1 change: 1 addition & 0 deletions defs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
* must not be greater than any pDNS system's concurrent connection limit.
*/
#define MAX_FETCHES 8
#define MAX_FETCH_BUF (16*1024*1024)

@cmikk cmikk Jun 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


#define DNSDBQ_SYSTEM "DNSDBQ_SYSTEM"

Expand Down
8 changes: 5 additions & 3 deletions dnsdbq.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
#include <errno.h>
#include <fcntl.h>
#include <inttypes.h>
#include <signal.h>
#include <stdarg.h>
#include <stdbool.h>
#include <stdio.h>
Expand Down Expand Up @@ -621,6 +620,8 @@ main(int argc, char *argv[]) {
usage("can't mix -g with -J");
if (qp.offset != 0)
usage("can't mix -O with -J");
if (presentation == pres_minimal)
usage("can't mix -p minimal with -J");
ruminate_json(json_fd, &qp);
close(json_fd);
} else if (batching != batch_none) {
Expand Down Expand Up @@ -978,7 +979,7 @@ qparam_option(int opt, const char *arg, qparam_t qpp) {
qpp->explicit_output_limit = qpp->output_limit;
break;
case 'O':
if (!parse_long(optarg, &qpp->offset) ||
if (!parse_long(arg, &qpp->offset) ||
(qpp->offset < 0))
return "-O must be zero or positive";
break;
Expand Down Expand Up @@ -1029,7 +1030,8 @@ select_config(void) {
for (conf = conf_files; *conf != NULL; conf++) {
wordexp_t we;

wordexp(*conf, &we, WRDE_NOCMD);
if (wordexp(*conf, &we, WRDE_NOCMD) != 0)
continue;
cf = strdup(we.we_wordv[0]);
wordfree(&we);
if (access(cf, R_OK) == 0) {
Expand Down
28 changes: 23 additions & 5 deletions netio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,11 @@ writer_init(long output_limit, ps_user_t ps_user, bool meta_query) {
exec_sort(p1, p2);
close(p1[0]);
writer->sort_stdin = fdopen(p1[1], "w");
if (writer->sort_stdin == NULL)
my_panic(true, "fdopen");
writer->sort_stdout = fdopen(p2[0], "r");
if (writer->sort_stdout == NULL)
my_panic(true, "fdopen");
close(p2[1]);
}

Expand Down Expand Up @@ -278,6 +282,7 @@ writer_func(char *ptr, size_t size, size_t nmemb, void *blob) {
writer_t writer = query->writer;
qparam_ct qp = &query->qp;
size_t bytes = size * nmemb;
char *x;

@cmikk cmikk Jun 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

char *nl;

DEBUG(3, true, "writer_func(%d, %d): %d\n",
Expand Down Expand Up @@ -306,7 +311,15 @@ writer_func(char *ptr, size_t size, size_t nmemb, void *blob) {
}
}

fetch->buf = realloc(fetch->buf, fetch->len + bytes);
if (fetch->len + bytes > MAX_FETCH_BUF) {
printf("?? very large response\n");

@cmikk cmikk Jun 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failure of this check should:

  1. use my_logf() to print a warning rather than printf()ing to the output, and:
  2. return CURL_WRITEFUNC_ERROR, since returning 0 to signal error is not robust.

return 0;
}
x = realloc(fetch->buf, fetch->len + bytes);
if (x == NULL)
my_panic(true, "realloc");
fetch->buf = x;
x = NULL;
memcpy(fetch->buf + fetch->len, ptr, bytes);
fetch->len += bytes;

Expand Down Expand Up @@ -365,8 +378,11 @@ writer_func(char *ptr, size_t size, size_t nmemb, void *blob) {
fetch->stopped = true;
} else if (writer->meta_query) {
/* concatenate this fragment (incl \n) to ps_buf. */
writer->ps_buf = realloc(writer->ps_buf,
writer->ps_len + pre_len + 1);
x = realloc(writer->ps_buf, writer->ps_len + pre_len + 1);
if (x == NULL)
my_panic(true, "realloc");
writer->ps_buf = x;
x = NULL;
memcpy(writer->ps_buf + writer->ps_len,
fetch->buf, pre_len + 1);
writer->ps_len += pre_len + 1;
Expand Down Expand Up @@ -431,11 +447,13 @@ last_fetch(fetch_t fetch) {
writer->active = NULL;
}
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",

@cmikk cmikk Jun 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

or_else(query->status, status_noerror),
or_else(query->message,
or_else(fetch->saf_msg, "no error")));
if (x < 0)
my_panic(true, "asprintf");
writer->ps_len = (size_t) x;
if (npaused > 0) {
query_t unpause;
fetch_t ufetch;
Expand Down
112 changes: 59 additions & 53 deletions pdns.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -644,7 +644,8 @@ tuple_make(pdns_tuple_t tup, const char *buf, size_t len) {
my_logf("warning: json_loadb: %d:%d: %s %s",
error.line, error.column,
error.text, error.source);
abort();
msg = "json_loadb failed";
goto ouch;
}
if (debug_level >= 4) {
char *pretty = json_dumps(tup->obj.main, JSON_INDENT(2));
Expand Down Expand Up @@ -762,58 +763,63 @@ tuple_make(pdns_tuple_t tup, const char *buf, size_t len) {

/* Records. */
tup->obj.rrname = json_object_get(tup->obj.cof_obj, "rrname");
if (tup->obj.rrname != NULL) {
if (!json_is_string(tup->obj.rrname)) {
msg = "rrname must be a string";
goto ouch;
}

char *r = strdup(json_string_value(tup->obj.rrname));
int dot = 0;

if ((transforms & TRANS_REVERSE) != 0) {
char *t = reverse(r);
DESTROY(r);
r = t;
t = NULL;
/* leading dot comes from reverse() */
if ((transforms & TRANS_CHOMP) != 0)
dot = 1;
} else if ((transforms & TRANS_CHOMP) != 0) {
/* unescaped trailing dot? */
size_t l = strlen(r);
if (l > 0 && r[l-1] == '.' &&
(l == 1 || r[l-2] != '\\'))
r[l-1] = '\0';
}

if (dot) {
/* in chomp+reverse, the dot to chomp is now leading. */
tup->rrname = strdup(r + dot);
DESTROY(r);
} else {
tup->rrname = r;
}
if (tup->obj.rrname == NULL) {
msg = "rrname is required";
goto ouch;
}
tup->obj.rrtype = json_object_get(tup->obj.cof_obj, "rrtype");
if (tup->obj.rrtype != NULL) {
if (!json_is_string(tup->obj.rrtype)) {
msg = "rrtype must be a string";
goto ouch;
}
tup->rrtype = json_string_value(tup->obj.rrtype);
}
tup->obj.rdata = json_object_get(tup->obj.cof_obj, "rdata");
if (tup->obj.rdata != NULL) {
if (json_is_string(tup->obj.rdata)) {
tup->rdata = json_string_value(tup->obj.rdata);
} else if (!json_is_array(tup->obj.rdata)) {
msg = "rdata must be a string or array";
goto ouch;
}
/* N.b., the array case is for the consumer to iterate over. */
if (!json_is_string(tup->obj.rrname)) {
msg = "rrname must be a string";
goto ouch;
}

char *r = strdup(json_string_value(tup->obj.rrname));
int dot = 0;

if ((transforms & TRANS_REVERSE) != 0) {
char *t = reverse(r);
DESTROY(r);
r = t;
t = NULL;
/* leading dot comes from reverse() */
if ((transforms & TRANS_CHOMP) != 0)
dot = 1;
} else if ((transforms & TRANS_CHOMP) != 0) {
/* unescaped trailing dot? */
size_t l = strlen(r);
if (l > 0 && r[l - 1] == '.' && (l == 1 || r[l - 2] != '\\'))
r[l - 1] = '\0';
}

if (dot) {
/* in chomp+reverse, the dot to chomp is now leading. */
tup->rrname = strdup(r + dot);
DESTROY(r);
} else {
tup->rrname = r;
}
tup->obj.rrtype = json_object_get(tup->obj.cof_obj, "rrtype");
if (tup->obj.rrtype == NULL) {
msg = "rrtype is required";
goto ouch;
}
if (!json_is_string(tup->obj.rrtype)) {
msg = "rrtype must be a string";
goto ouch;
}
tup->rrtype = json_string_value(tup->obj.rrtype);
tup->obj.rdata = json_object_get(tup->obj.cof_obj, "rdata");
if (tup->obj.rdata == NULL) {
msg = "rdata is required";
goto ouch;
}
if (json_is_string(tup->obj.rdata)) {
tup->rdata = json_string_value(tup->obj.rdata);
} else if (!json_is_array(tup->obj.rdata)) {
msg = "rdata must be a string or array";
goto ouch;
}
/* N.b., the array case is for the consumer to iterate over. */

assert(msg == NULL);
return NULL;

Expand Down Expand Up @@ -1089,7 +1095,7 @@ pick_system(const char *name, const char *context) {
}

if (msg != NULL) {
my_logf("%s (in %s)\n", msg, context);
my_logf("%s (in %s)", msg, context);
DESTROY(msg);
my_exit(1);
}
Expand Down Expand Up @@ -1145,7 +1151,7 @@ read_config(void) {

l++;
if (strchr(line, '\n') == NULL) {
my_logf("conf line #%d: too long", l);
my_logf("conf line #%d: indecipherable", l);
my_exit(1);
}
tok1 = strtok_r(line, "\040\012", &saveptr);
Expand All @@ -1166,7 +1172,7 @@ read_config(void) {
if (strcmp(tok2, "system") == 0 && !psys_specified) {
pick_system(tok3, config_file);
if (psys == NULL) {
my_logf("unknown %s %s\n",
my_logf("unknown %s %s",
DNSDBQ_SYSTEM,
tok3);
my_exit(1);
Expand Down
3 changes: 2 additions & 1 deletion pdns_dnsdb.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,8 @@ rate_tuple_make(rate_tuple_t tup, const char *buf, size_t len) {
my_logf("warning: json_loadb: %d:%d: %s %s",
error.line, error.column,
error.text, error.source);
abort();
msg = "json_loadb failed";
goto ouch;
}
if (debug_level >= 4) {
char *pretty = json_dumps(tup->obj.main, JSON_INDENT(2));
Expand Down
3 changes: 1 addition & 2 deletions time.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,7 @@ time_get(const char *src, u_long *dst) {
ll = strtoll(src, &ep, 10);
if (*src != '\0' && *ep == '\0') {
if (ll < 0)
*dst = (u_long)startup_time.tv_sec -
(u_long)imaxabs(ll);
*dst = (u_long)(startup_time.tv_sec + ll);
else
*dst = (u_long)ll;
return 1;
Expand Down
3 changes: 0 additions & 3 deletions tokstr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,6 @@ struct tokstr *tokstr_region(struct tokstr_reg);
// tokstr_string -- create an iterator for a nul-terminated string
struct tokstr *tokstr_string(const char *);

// tokstr_string -- create an iterator for a nul-terminated string
struct tokstr *tokstr_string(const char *);

// tokstr_next -- return next token from an iterator (which must be free()'d)
// (NULL means no more tokens are available.)
char *tokstr_next(struct tokstr *, const char *);
Expand Down