Changeset View
Changeset View
Standalone View
Standalone View
src/seeder/dns.cpp
Show First 20 Lines • Show All 68 Lines • ▼ Show 20 Lines | int parse_name(const uint8_t **inpos, const uint8_t *inend, | ||||
} | } | ||||
size_t bufused = 0; | size_t bufused = 0; | ||||
int init = 1; | int init = 1; | ||||
do { | do { | ||||
if (*inpos == inend) { | if (*inpos == inend) { | ||||
return -1; | return -1; | ||||
} | } | ||||
// read length of next component | // read length of next component | ||||
int octet = *((*inpos)++); | unsigned int octet = *((*inpos)++); | ||||
Fabien: Whatever the type is, if you go from signed to unsigned you need to be sure that it is safe… | |||||
nakihitoAuthorUnsubmitted Done Inline ActionsThese changes were made to answer some type comparison mismatch compiler warnings. I have determined the less intrusive change is to change the typing of the constants instead. nakihito: These changes were made to answer some type comparison mismatch compiler warnings. I have… | |||||
if (octet == 0) { | if (octet == 0) { | ||||
buf[bufused] = 0; | buf[bufused] = 0; | ||||
return 0; | return 0; | ||||
} | } | ||||
// add dot in output | // add dot in output | ||||
if (!init) { | if (!init) { | ||||
// The maximum size of a query name is 255. The buffer must have | // The buffer must have room for at least the '.', a valid | ||||
// room for at least the '.' and a valid non-'.' character | // non-'.' character, and a null-terminating character. | ||||
if (bufused > 253) { | if (bufused > MAX_QUERY_NAME_BUFFER_LENGTH - 3) { | ||||
return -1; | return -1; | ||||
} | } | ||||
if (bufused == bufsize - 1) { | if (bufused == bufsize - 1) { | ||||
return -2; | return -2; | ||||
} | } | ||||
buf[bufused++] = '.'; | buf[bufused++] = '.'; | ||||
} else { | } else { | ||||
init = 0; | init = 0; | ||||
} | } | ||||
// handle references | // handle references | ||||
if ((octet & 0xC0) == 0xC0) { | if ((octet & 0xC0) == 0xC0) { | ||||
if (*inpos == inend) { | if (*inpos == inend) { | ||||
return -1; | return -1; | ||||
} | } | ||||
int ref = ((octet - 0xC0) << 8) + *((*inpos)++); | int ref = ((octet - 0xC0) << 8) + *((*inpos)++); | ||||
if (ref < 0 || ref >= (*inpos) - inbuf - 2) { | if (ref < 0 || ref >= (*inpos) - inbuf - 2) { | ||||
return -1; | return -1; | ||||
} | } | ||||
const uint8_t *newbuf = inbuf + ref; | const uint8_t *newbuf = inbuf + ref; | ||||
return parse_name(&newbuf, (*inpos) - 2, inbuf, buf + bufused, | return parse_name(&newbuf, (*inpos) - 2, inbuf, buf + bufused, | ||||
bufsize - bufused); | bufsize - bufused); | ||||
} | } | ||||
if (octet > 63) { | if (octet > MAX_LABEL_LENGTH) { | ||||
return -1; | return -1; | ||||
} | } | ||||
// copy label | // copy label | ||||
while (octet) { | while (octet) { | ||||
if (*inpos == inend || bufused > 254) { | if (*inpos == inend || bufused > MAX_QUERY_NAME_LENGTH - 1) { | ||||
return -1; | return -1; | ||||
} | } | ||||
if (bufused == bufsize - 1) { | if (bufused == bufsize - 1) { | ||||
return -2; | return -2; | ||||
} | } | ||||
int c = *((*inpos)++); | int c = *((*inpos)++); | ||||
if (c == '.') { | if (c == '.') { | ||||
return -1; | return -1; | ||||
} | } | ||||
octet--; | octet--; | ||||
buf[bufused++] = c; | buf[bufused++] = c; | ||||
} | } | ||||
} while (1); | } while (1); | ||||
} | } | ||||
// 0: k | // 0: k | ||||
FabienUnsubmitted Not Done Inline ActionsMind changing k to ok while you're at it ? Fabien: Mind changing `k` to `ok` while you're at it ? | |||||
// -1: component > 63 characters | // -1: label > MAX_LABEL_LENGTH characters | ||||
// -2: insufficent space in output | // -2: insufficent space in output | ||||
// -3: two subsequent dots | // -3: two subsequent dots | ||||
static int write_name(uint8_t **outpos, const uint8_t *outend, const char *name, | static int write_name(uint8_t **outpos, const uint8_t *outend, const char *name, | ||||
int offset) { | int offset) { | ||||
while (*name != 0) { | while (*name != 0) { | ||||
const char *dot = strchr(name, '.'); | const char *dot = strchr(name, '.'); | ||||
const char *fin = dot; | const char *fin = dot; | ||||
if (!dot) { | if (!dot) { | ||||
fin = name + strlen(name); | fin = name + strlen(name); | ||||
} | } | ||||
if (fin - name > 63) { | unsigned int labelLength = fin - name; | ||||
FabienUnsubmitted Not Done Inline ActionsThere is a type for pointer arithmetic: https://en.cppreference.com/w/cpp/types/ptrdiff_t Fabien: There is a type for pointer arithmetic: https://en.cppreference.com/w/cpp/types/ptrdiff_t | |||||
nakihitoAuthorUnsubmitted Done Inline ActionsI'll leave this for a different diff. nakihito: I'll leave this for a different diff. | |||||
if (labelLength > MAX_LABEL_LENGTH) { | |||||
return -1; | return -1; | ||||
} | } | ||||
if (fin == name) { | if (fin == name) { | ||||
return -3; | return -3; | ||||
} | } | ||||
if (outend - *outpos < fin - name + 2) { | if (outend - *outpos < labelLength + 2) { | ||||
return -2; | return -2; | ||||
} | } | ||||
*((*outpos)++) = fin - name; | *((*outpos)++) = labelLength; | ||||
memcpy(*outpos, name, fin - name); | memcpy(*outpos, name, labelLength); | ||||
*outpos += fin - name; | *outpos += labelLength; | ||||
if (!dot) { | if (!dot) { | ||||
break; | break; | ||||
} | } | ||||
name = dot + 1; | name = dot + 1; | ||||
} | } | ||||
if (offset < 0) { | if (offset < 0) { | ||||
// no reference | // no reference | ||||
if (outend == *outpos) { | if (outend == *outpos) { | ||||
▲ Show 20 Lines • Show All 254 Lines • ▼ Show 20 Lines | if (nquestion > 1) { | ||||
/* fprintf(stdout, "Multiple questions %i?\n", nquestion); */ | /* fprintf(stdout, "Multiple questions %i?\n", nquestion); */ | ||||
responseCode = DNSResponseCode::NOT_IMPLEMENTED; | responseCode = DNSResponseCode::NOT_IMPLEMENTED; | ||||
goto error; | goto error; | ||||
} | } | ||||
{ | { | ||||
const uint8_t *inpos = inbuf + 12; | const uint8_t *inpos = inbuf + 12; | ||||
const uint8_t *inend = inbuf + insize; | const uint8_t *inend = inbuf + insize; | ||||
char name[256]; | char name[MAX_QUERY_NAME_BUFFER_LENGTH]; | ||||
int offset = inpos - inbuf; | int offset = inpos - inbuf; | ||||
int ret = parse_name(&inpos, inend, inbuf, name, 256); | int ret = parse_name(&inpos, inend, inbuf, name, | ||||
MAX_QUERY_NAME_BUFFER_LENGTH); | |||||
if (ret == -1) { | if (ret == -1) { | ||||
responseCode = DNSResponseCode::FORMAT_ERROR; | responseCode = DNSResponseCode::FORMAT_ERROR; | ||||
goto error; | goto error; | ||||
} | } | ||||
if (ret == -2) { | if (ret == -2) { | ||||
responseCode = DNSResponseCode::REFUSED; | responseCode = DNSResponseCode::REFUSED; | ||||
goto error; | goto error; | ||||
▲ Show 20 Lines • Show All 246 Lines • Show Last 20 Lines |
Whatever the type is, if you go from signed to unsigned you need to be sure that it is safe when using the - operator (which is done a few lines below) because this is a behavior change.
What is the rational behind this change (which seems unrelated to what the summary describe as the intent for this diff) ?