Merge branch 'aram/cfg_print_duration-uninitialized-length' into 'main'

Fix a logical bug in cfg_print_duration()

See merge request isc-projects/bind9!6880
This commit is contained in:
Arаm Sаrgsyаn 2022-10-17 09:15:13 +00:00
commit cf230dea7a
5 changed files with 163 additions and 46 deletions

View file

@ -1,3 +1,8 @@
5996. [bug] Fix a couple of bugs in cfg_print_duration(), which
could result in generating incomplete duration values
when printing the configuration using named-checkconf.
[GL !6880]
5995. [performance] A new algorithm for DNS name compression based on a
hash set of message offsets. Name compression is now
more complete as well as being generally faster, and

View file

@ -17,6 +17,7 @@
#include <errno.h>
#include <inttypes.h>
#include <stdbool.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
@ -39,14 +40,14 @@
isc_result_t
isccfg_duration_fromtext(isc_textregion_t *source,
isccfg_duration_t *duration) {
char buf[DURATION_MAXLEN];
char buf[CFG_DURATION_MAXLEN] = { 0 };
char *P, *X, *T, *W, *str;
bool not_weeks = false;
int i;
long long int lli;
/*
* Copy the buffer as it may not be NULL terminated.
* Anyone having a duration longer than 63 characters is crazy.
*/
if (source->length > sizeof(buf) - 1) {
return (ISC_R_BADNUMBER);
@ -74,7 +75,12 @@ isccfg_duration_fromtext(isc_textregion_t *source,
/* Record years. */
X = strpbrk(str, "Yy");
if (X != NULL) {
duration->parts[0] = atoi(str + 1);
errno = 0;
lli = strtoll(str + 1, NULL, 10);
if (errno != 0 || lli < 0 || lli > UINT32_MAX) {
return (ISC_R_BADNUMBER);
}
duration->parts[0] = (uint32_t)lli;
str = X;
not_weeks = true;
}
@ -87,7 +93,12 @@ isccfg_duration_fromtext(isc_textregion_t *source,
* part, or this M indicator is before the time indicator.
*/
if (X != NULL && (T == NULL || (size_t)(X - P) < (size_t)(T - P))) {
duration->parts[1] = atoi(str + 1);
errno = 0;
lli = strtoll(str + 1, NULL, 10);
if (errno != 0 || lli < 0 || lli > UINT32_MAX) {
return (ISC_R_BADNUMBER);
}
duration->parts[1] = (uint32_t)lli;
str = X;
not_weeks = true;
}
@ -95,7 +106,12 @@ isccfg_duration_fromtext(isc_textregion_t *source,
/* Record days. */
X = strpbrk(str, "Dd");
if (X != NULL) {
duration->parts[3] = atoi(str + 1);
errno = 0;
lli = strtoll(str + 1, NULL, 10);
if (errno != 0 || lli < 0 || lli > UINT32_MAX) {
return (ISC_R_BADNUMBER);
}
duration->parts[3] = (uint32_t)lli;
str = X;
not_weeks = true;
}
@ -109,7 +125,12 @@ isccfg_duration_fromtext(isc_textregion_t *source,
/* Record hours. */
X = strpbrk(str, "Hh");
if (X != NULL && T != NULL) {
duration->parts[4] = atoi(str + 1);
errno = 0;
lli = strtoll(str + 1, NULL, 10);
if (errno != 0 || lli < 0 || lli > UINT32_MAX) {
return (ISC_R_BADNUMBER);
}
duration->parts[4] = (uint32_t)lli;
str = X;
not_weeks = true;
}
@ -122,7 +143,12 @@ isccfg_duration_fromtext(isc_textregion_t *source,
* part and the M indicator is behind the time indicator.
*/
if (X != NULL && T != NULL && (size_t)(X - P) > (size_t)(T - P)) {
duration->parts[5] = atoi(str + 1);
errno = 0;
lli = strtoll(str + 1, NULL, 10);
if (errno != 0 || lli < 0 || lli > UINT32_MAX) {
return (ISC_R_BADNUMBER);
}
duration->parts[5] = (uint32_t)lli;
str = X;
not_weeks = true;
}
@ -130,7 +156,12 @@ isccfg_duration_fromtext(isc_textregion_t *source,
/* Record seconds. */
X = strpbrk(str, "Ss");
if (X != NULL && T != NULL) {
duration->parts[6] = atoi(str + 1);
errno = 0;
lli = strtoll(str + 1, NULL, 10);
if (errno != 0 || lli < 0 || lli > UINT32_MAX) {
return (ISC_R_BADNUMBER);
}
duration->parts[6] = (uint32_t)lli;
str = X;
not_weeks = true;
}
@ -142,7 +173,12 @@ isccfg_duration_fromtext(isc_textregion_t *source,
/* Mix of weeks and other indicators is not allowed */
return (ISC_R_BADNUMBER);
} else {
duration->parts[2] = atoi(str + 1);
errno = 0;
lli = strtoll(str + 1, NULL, 10);
if (errno != 0 || lli < 0 || lli > UINT32_MAX) {
return (ISC_R_BADNUMBER);
}
duration->parts[2] = (uint32_t)lli;
str = W;
}
}
@ -183,20 +219,21 @@ isccfg_parse_duration(isc_textregion_t *source, isccfg_duration_t *duration) {
uint32_t
isccfg_duration_toseconds(const isccfg_duration_t *duration) {
uint32_t seconds = 0;
uint64_t seconds = 0;
REQUIRE(duration != NULL);
seconds += duration->parts[6]; /* Seconds */
seconds += duration->parts[5] * 60; /* Minutes */
seconds += duration->parts[4] * 3600; /* Hours */
seconds += duration->parts[3] * 86400; /* Days */
seconds += duration->parts[2] * 86400 * 7; /* Weeks */
seconds += (uint64_t)duration->parts[6]; /* Seconds */
seconds += (uint64_t)duration->parts[5] * 60; /* Minutes */
seconds += (uint64_t)duration->parts[4] * 3600; /* Hours */
seconds += (uint64_t)duration->parts[3] * 86400; /* Days */
seconds += (uint64_t)duration->parts[2] * 86400 * 7; /* Weeks */
/*
* The below additions are not entirely correct
* because days may very per month and per year.
* because days may vary per month and per year.
*/
seconds += duration->parts[1] * 86400 * 31; /* Months */
seconds += duration->parts[0] * 86400 * 365; /* Years */
return (seconds);
seconds += (uint64_t)duration->parts[1] * 86400 * 31; /* Months */
seconds += (uint64_t)duration->parts[0] * 86400 * 365; /* Years */
return (seconds > UINT32_MAX ? UINT32_MAX : (uint32_t)seconds);
}

View file

@ -24,7 +24,7 @@
ISC_LANG_BEGINDECLS
#define DURATION_MAXLEN 64
#define CFG_DURATION_MAXLEN 80
/*%
* A configuration object to store ISO 8601 durations.
@ -76,6 +76,10 @@ isccfg_duration_toseconds(const isccfg_duration_t *duration);
* - Months will be treated as 31 days.
* - Years will be treated as 365 days.
*
* Notes:
*\li If the duration in seconds is greater than UINT32_MAX, the return value
* will be UINT32_MAX.
*
* Returns:
*\li The duration in seconds.
*/

View file

@ -1030,11 +1030,11 @@ numlen(uint32_t num) {
*/
void
cfg_print_duration(cfg_printer_t *pctx, const cfg_obj_t *obj) {
char buf[DURATION_MAXLEN];
char buf[CFG_DURATION_MAXLEN];
char *str;
const char *indicators = "YMWDHMS";
int count, i;
int durationlen[7];
int durationlen[7] = { 0 };
isccfg_duration_t duration;
/*
* D ? The duration has a date part.
@ -1066,10 +1066,8 @@ cfg_print_duration(cfg_printer_t *pctx, const cfg_obj_t *obj) {
} else {
T = true;
}
} else {
durationlen[i] = 0;
count += durationlen[i];
}
count += durationlen[i];
}
/*
* Special case for seconds which is not taken into account in the
@ -1087,7 +1085,7 @@ cfg_print_duration(cfg_printer_t *pctx, const cfg_obj_t *obj) {
if (T) {
count++;
}
INSIST(count < DURATION_MAXLEN);
INSIST(count < CFG_DURATION_MAXLEN);
/* Now print the duration. */
for (i = 0; i < 6; i++) {
@ -1098,7 +1096,7 @@ cfg_print_duration(cfg_printer_t *pctx, const cfg_obj_t *obj) {
if (duration.parts[i] > 0) {
snprintf(str, durationlen[i] + 2, "%u%c",
(uint32_t)duration.parts[i], indicators[i]);
str += durationlen[i] + 1;
str += durationlen[i];
}
if (i == 3 && T) {
snprintf(str, 2, "T");
@ -1107,7 +1105,7 @@ cfg_print_duration(cfg_printer_t *pctx, const cfg_obj_t *obj) {
}
/* Special case for seconds. */
if (duration.parts[6] > 0 ||
(!D && !duration.parts[4] && !duration.parts[3])) {
(!D && !duration.parts[4] && !duration.parts[5])) {
snprintf(str, durationlen[6] + 2, "%u%c",
(uint32_t)duration.parts[6], indicators[6]);
}

View file

@ -15,6 +15,7 @@
#include <setjmp.h>
#include <stdarg.h>
#include <stddef.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
@ -84,17 +85,34 @@ ISC_TEARDOWN_TEST_IMPL(group) {
struct duration_conf {
const char *string;
uint32_t time;
const char *out;
};
typedef struct duration_conf duration_conf_t;
/* test cfg_obj_asduration() */
ISC_RUN_TEST_IMPL(cfg_obj_asduration) {
static void
output(void *closure, const char *text, int textlen) {
int r;
REQUIRE(textlen >= 0 && textlen < CFG_DURATION_MAXLEN);
r = snprintf(closure, CFG_DURATION_MAXLEN, "%s", text);
INSIST(r == textlen);
}
/* test cfg_obj_asduration() and cfg_print_duration() */
ISC_RUN_TEST_IMPL(duration) {
isc_result_t result;
/*
* When 'out' is NULL, the printed result is expected to be the same as
* the input 'string', compared by ignoring the case of the characters.
*/
duration_conf_t durations[] = {
{ .string = "unlimited", .time = 0 },
{ .string = "PT0S", .time = 0 },
{ .string = "PT42S", .time = 42 },
{ .string = "PT10m", .time = 600 },
{ .string = "PT10m4S", .time = 604 },
{ .string = "PT3600S", .time = 3600 },
{ .string = "pT2H", .time = 7200 },
{ .string = "Pt2H3S", .time = 7203 },
{ .string = "PT2h1m3s", .time = 7263 },
@ -106,26 +124,56 @@ ISC_RUN_TEST_IMPL(cfg_obj_asduration) {
{ .string = "p5y", .time = 157680000 },
{ .string = "P5YT2H", .time = 157687200 },
{ .string = "P1Y1M1DT1H1M1S", .time = 34304461 },
{ .string = "P99Y399M999DT3999H9999M2911754S",
.time = UINT32_MAX - 1 },
{ .string = "P99Y399M999DT3999H9999M2911755S",
.time = UINT32_MAX },
{ .string = "P4294967295Y4294967295M4294967295D"
"T4294967295H4294967295M4294967295S",
.time = UINT32_MAX },
{ .string = "PT4294967294S", .time = UINT32_MAX - 1 },
{ .string = "PT4294967295S", .time = UINT32_MAX },
{ .string = "0", .time = 0 },
{ .string = "30", .time = 30 },
{ .string = "42s", .time = 42 },
{ .string = "10m", .time = 600 },
{ .string = "2H", .time = 7200 },
{ .string = "7d", .time = 604800 },
{ .string = "2w", .time = 1209600 },
{ .string = "42s", .time = 42, .out = "42" },
{ .string = "10m", .time = 600, .out = "600" },
{ .string = "2H", .time = 7200, .out = "7200" },
{ .string = "7d", .time = 604800, .out = "604800" },
{ .string = "2w", .time = 1209600, .out = "1209600" },
{ 0 }, /* Indicates that the remaining durations are invalid. */
{ .string = "PT4Y" },
{ .string = "P-4Y2M" },
{ .string = "P5H1M30S" },
{ .string = "P7Y4W" },
{ .string = "X7Y4M" },
{ .string = "T7H4M" },
{ .string = "1Y6M" },
{ .string = "PT4294967296S" },
{ .string = "PT99999999999S" },
{ .string = "P99999999999Y99999999999M99999999999D"
"T99999999999H99999999999M99999999999S" },
};
int num = 22;
isc_buffer_t buf1;
cfg_parser_t *p1 = NULL;
cfg_obj_t *c1 = NULL;
bool must_fail = false;
for (int i = 0; i < num; i++) {
for (size_t i = 0; i < ARRAY_SIZE(durations); i++) {
const cfg_listelt_t *element;
const cfg_obj_t *kasps = NULL;
char conf[64];
sprintf(&conf[0],
"dnssec-policy \"dp\"\n{\nsignatures-refresh %s;\n};\n",
durations[i].string);
const char cfg_tpl[] =
"dnssec-policy \"dp\"\n"
"{\nkeys {csk lifetime %s algorithm rsasha256;};\n};\n";
char conf[sizeof(cfg_tpl) + CFG_DURATION_MAXLEN] = { 0 };
char out[CFG_DURATION_MAXLEN] = { 0 };
cfg_printer_t pctx = { .f = output, .closure = out };
if (durations[i].string == NULL) {
must_fail = true;
continue;
}
snprintf(&conf[0], sizeof(conf), cfg_tpl, durations[i].string);
isc_buffer_init(&buf1, conf, strlen(conf) - 1);
isc_buffer_add(&buf1, strlen(conf) - 1);
@ -136,6 +184,11 @@ ISC_RUN_TEST_IMPL(cfg_obj_asduration) {
result = cfg_parse_buffer(p1, &buf1, "text1", 0,
&cfg_type_namedconf, 0, &c1);
if (must_fail) {
assert_int_equal(result, DNS_R_BADTTL);
cfg_parser_destroy(&p1);
continue;
}
assert_int_equal(result, ISC_R_SUCCESS);
(void)cfg_map_get(c1, "dnssec-policy", &kasps);
@ -143,17 +196,37 @@ ISC_RUN_TEST_IMPL(cfg_obj_asduration) {
for (element = cfg_list_first(kasps); element != NULL;
element = cfg_list_next(element))
{
const cfg_obj_t *d1 = NULL;
const cfg_listelt_t *key_element;
const cfg_obj_t *lifetime = NULL;
const cfg_obj_t *keys = NULL;
const cfg_obj_t *key = NULL;
const cfg_obj_t *kopts = NULL;
cfg_obj_t *kconf = cfg_listelt_value(element);
int cmp;
assert_non_null(kconf);
kopts = cfg_tuple_get(kconf, "options");
result = cfg_map_get(kopts, "signatures-refresh", &d1);
assert_int_equal(result, ISC_R_SUCCESS);
result = cfg_map_get(kopts, "keys", &keys);
key_element = cfg_list_first(keys);
assert_non_null(key_element);
key = cfg_listelt_value(key_element);
assert_non_null(key);
lifetime = cfg_tuple_get(key, "lifetime");
assert_non_null(lifetime);
assert_int_equal(durations[i].time,
cfg_obj_asduration(d1));
cfg_obj_asduration(lifetime));
cfg_print_duration_or_unlimited(&pctx, lifetime);
cmp = strncasecmp(durations[i].out != NULL
? durations[i].out
: durations[i].string,
out, strlen(durations[i].string));
assert_int_equal(cmp, 0);
}
cfg_obj_destroy(p1, &c1);
@ -163,7 +236,7 @@ ISC_RUN_TEST_IMPL(cfg_obj_asduration) {
ISC_TEST_LIST_START
ISC_TEST_ENTRY(cfg_obj_asduration)
ISC_TEST_ENTRY(duration)
ISC_TEST_LIST_END