* Merging ns_parse from Alpine
@ 2014-12-14 0:43 Rich Felker
2014-12-14 7:38 ` Felix Janda
0 siblings, 1 reply; 5+ messages in thread
From: Rich Felker @ 2014-12-14 0:43 UTC (permalink / raw)
To: musl
[-- Attachment #1: Type: text/plain, Size: 1022 bytes --]
I'm working on merging Timo's patch for ns_parse:
http://git.alpinelinux.org/cgit/aports/tree/main/musl/1001-add-basic-dns-record-parsing-functions.patch?id=81d50064c335467fdfd80368bac6707d70db1af7
The first issue that came up in the process is that arpa/nameser.h,
which was previously not used by musl itself and really should never
have been accepted in its current form, is full of junk like
statement-expressions. Including it in a file that will be compiled
with musl adds build dependency on these nonstandard features. I
cleaned that up with no problem (just un-inlining the macros since
we're adding function versions anyway), but there are a few more
issues.
The main issue is that the parser functions have pointer arithmetic
overflows (UB) checking against the end-of-message pointer. I've tried
to fix that and I'm attaching a patch for review, along with my
version of the fixed file. I'd appreciate comments on whether I missed
anything.
Other changes were mostly cosmetic or at least mechanical.
Rich
[-- Attachment #2: ns_parse_changes_v1.diff --]
[-- Type: text/plain, Size: 3504 bytes --]
--- src/network/ns_parse.c.orig
+++ src/network/ns_parse.c
@@ -23,28 +23,28 @@
{ 0x0000, 0 },
};
-u_int ns_get16(const unsigned char *cp)
+unsigned ns_get16(const unsigned char *cp)
{
- u_short s;
- NS_GET16(s, cp);
- return s;
+ return cp[0]<<8 | cp[1];
}
-u_long ns_get32(const unsigned char *cp)
+unsigned long ns_get32(const unsigned char *cp)
{
- u_long l;
- NS_GET32(l, cp);
- return l;
+ return (unsigned)cp[0]<<24 | cp[1]<<16 | cp[2]<<8 | cp[3];
}
-void ns_put16(u_int s, unsigned char *cp)
+void ns_put16(unsigned s, unsigned char *cp)
{
- NS_PUT16(s, cp);
+ *cp++ = s>>8;
+ *cp++ = s;
}
-void ns_put32(u_long l, unsigned char *cp)
+void ns_put32(unsigned long l, unsigned char *cp)
{
- NS_PUT32(l, cp);
+ *cp++ = l>>24;
+ *cp++ = l>>16;
+ *cp++ = l>>8;
+ *cp++ = l;
}
int ns_initparse(const unsigned char *msg, int msglen, ns_msg *handle)
@@ -56,8 +56,11 @@
if (msglen < (2 + ns_s_max) * NS_INT16SZ) goto bad;
NS_GET16(handle->_id, msg);
NS_GET16(handle->_flags, msg);
- for (i = 0; i < ns_s_max; i++) NS_GET16(handle->_counts[i], msg);
for (i = 0; i < ns_s_max; i++) {
+ if (NS_INT16SZ > handle->_eom - msg) goto bad;
+ NS_GET16(handle->_counts[i], msg);
+ }
+ for (i = 0; i < ns_s_max; i++) {
if (handle->_counts[i]) {
handle->_sections[i] = msg;
r = ns_skiprr(msg, handle->_eom, i, handle->_counts[i]);
@@ -77,23 +80,24 @@
return -1;
}
-int ns_skiprr(const u_char *ptr, const u_char *eom, ns_sect section, int count)
+int ns_skiprr(const unsigned char *ptr, const unsigned char *eom, ns_sect section, int count)
{
- const u_char *p = ptr;
+ const unsigned char *p = ptr;
int r;
while (count--) {
r = dn_skipname(p, eom);
if (r < 0) goto bad;
+ if (r + 2 * NS_INT16SZ > eom - p) goto bad;
p += r + 2 * NS_INT16SZ;
if (section != ns_s_qd) {
- if (p + NS_INT32SZ + NS_INT16SZ > eom) goto bad;
+ if (NS_INT32SZ + NS_INT16SZ > eom - p) goto bad;
p += NS_INT32SZ;
NS_GET16(r, p);
+ if (r > eom - p) goto bad;
p += r;
}
}
- if (p > eom) goto bad;
return ptr - p;
bad:
errno = EMSGSIZE;
@@ -125,14 +129,14 @@
r = dn_expand(handle->_msg, handle->_eom, handle->_msg_ptr, rr->name, NS_MAXDNAME);
if (r < 0) return -1;
handle->_msg_ptr += r;
- if (handle->_msg_ptr + 2 * NS_INT16SZ > handle->_eom) goto size;
+ if (2 * NS_INT16SZ > handle->_eom - handle->_msg_ptr) goto size;
NS_GET16(rr->type, handle->_msg_ptr);
NS_GET16(rr->rr_class, handle->_msg_ptr);
if (section != ns_s_qd) {
- if (handle->_msg_ptr + NS_INT32SZ + NS_INT16SZ > handle->_eom) goto size;
+ if (NS_INT32SZ + NS_INT16SZ > handle->_eom - handle->_msg_ptr) goto size;
NS_GET32(rr->ttl, handle->_msg_ptr);
NS_GET16(rr->rdlength, handle->_msg_ptr);
- if (handle->_msg_ptr + rr->rdlength > handle->_eom) goto size;
+ if (rr->rdlength > handle->_eom - handle->_msg_ptr) goto size;
rr->rdata = handle->_msg_ptr;
handle->_msg_ptr += rr->rdlength;
} else {
@@ -159,13 +163,11 @@
return -1;
}
-int __dn_expand(const unsigned char *, const unsigned char *, const unsigned char *, char *, int);
-
-int ns_name_uncompress(const u_char *msg, const u_char *eom,
- const u_char *src, char *dst, size_t dstsiz)
+int ns_name_uncompress(const unsigned char *msg, const unsigned char *eom,
+ const unsigned char *src, char *dst, size_t dstsiz)
{
int r;
- r = __dn_expand(msg, eom, src, dst, dstsiz);
+ r = dn_expand(msg, eom, src, dst, dstsiz);
if (r < 0) errno = EMSGSIZE;
return r;
}
[-- Attachment #3: ns_parse.c --]
[-- Type: text/plain, Size: 3965 bytes --]
#define _BSD_SOURCE
#include <errno.h>
#include <stddef.h>
#include <resolv.h>
#include <arpa/nameser.h>
const struct _ns_flagdata _ns_flagdata[16] = {
{ 0x8000, 15 },
{ 0x7800, 11 },
{ 0x0400, 10 },
{ 0x0200, 9 },
{ 0x0100, 8 },
{ 0x0080, 7 },
{ 0x0040, 6 },
{ 0x0020, 5 },
{ 0x0010, 4 },
{ 0x000f, 0 },
{ 0x0000, 0 },
{ 0x0000, 0 },
{ 0x0000, 0 },
{ 0x0000, 0 },
{ 0x0000, 0 },
{ 0x0000, 0 },
};
unsigned ns_get16(const unsigned char *cp)
{
return cp[0]<<8 | cp[1];
}
unsigned long ns_get32(const unsigned char *cp)
{
return (unsigned)cp[0]<<24 | cp[1]<<16 | cp[2]<<8 | cp[3];
}
void ns_put16(unsigned s, unsigned char *cp)
{
*cp++ = s>>8;
*cp++ = s;
}
void ns_put32(unsigned long l, unsigned char *cp)
{
*cp++ = l>>24;
*cp++ = l>>16;
*cp++ = l>>8;
*cp++ = l;
}
int ns_initparse(const unsigned char *msg, int msglen, ns_msg *handle)
{
int i, r;
handle->_msg = msg;
handle->_eom = msg + msglen;
if (msglen < (2 + ns_s_max) * NS_INT16SZ) goto bad;
NS_GET16(handle->_id, msg);
NS_GET16(handle->_flags, msg);
for (i = 0; i < ns_s_max; i++) {
if (NS_INT16SZ > handle->_eom - msg) goto bad;
NS_GET16(handle->_counts[i], msg);
}
for (i = 0; i < ns_s_max; i++) {
if (handle->_counts[i]) {
handle->_sections[i] = msg;
r = ns_skiprr(msg, handle->_eom, i, handle->_counts[i]);
if (r < 0) return -1;
msg += r;
} else {
handle->_sections[i] = NULL;
}
}
if (msg != handle->_eom) goto bad;
handle->_sect = ns_s_max;
handle->_rrnum = -1;
handle->_msg_ptr = NULL;
return 0;
bad:
errno = EMSGSIZE;
return -1;
}
int ns_skiprr(const unsigned char *ptr, const unsigned char *eom, ns_sect section, int count)
{
const unsigned char *p = ptr;
int r;
while (count--) {
r = dn_skipname(p, eom);
if (r < 0) goto bad;
if (r + 2 * NS_INT16SZ > eom - p) goto bad;
p += r + 2 * NS_INT16SZ;
if (section != ns_s_qd) {
if (NS_INT32SZ + NS_INT16SZ > eom - p) goto bad;
p += NS_INT32SZ;
NS_GET16(r, p);
if (r > eom - p) goto bad;
p += r;
}
}
return ptr - p;
bad:
errno = EMSGSIZE;
return -1;
}
int ns_parserr(ns_msg *handle, ns_sect section, int rrnum, ns_rr *rr)
{
int r;
if (section < 0 || section >= ns_s_max) goto bad;
if (section != handle->_sect) {
handle->_sect = section;
handle->_rrnum = 0;
handle->_msg_ptr = handle->_sections[section];
}
if (rrnum == -1) rrnum = handle->_rrnum;
if (rrnum < 0 || rrnum >= handle->_counts[section]) goto bad;
if (rrnum < handle->_rrnum) {
handle->_rrnum = 0;
handle->_msg_ptr = handle->_sections[section];
}
if (rrnum > handle->_rrnum) {
r = ns_skiprr(handle->_msg_ptr, handle->_eom, section, rrnum - handle->_rrnum);
if (r < 0) return -1;
handle->_msg_ptr += r;
handle->_rrnum = rrnum;
}
r = dn_expand(handle->_msg, handle->_eom, handle->_msg_ptr, rr->name, NS_MAXDNAME);
if (r < 0) return -1;
handle->_msg_ptr += r;
if (2 * NS_INT16SZ > handle->_eom - handle->_msg_ptr) goto size;
NS_GET16(rr->type, handle->_msg_ptr);
NS_GET16(rr->rr_class, handle->_msg_ptr);
if (section != ns_s_qd) {
if (NS_INT32SZ + NS_INT16SZ > handle->_eom - handle->_msg_ptr) goto size;
NS_GET32(rr->ttl, handle->_msg_ptr);
NS_GET16(rr->rdlength, handle->_msg_ptr);
if (rr->rdlength > handle->_eom - handle->_msg_ptr) goto size;
rr->rdata = handle->_msg_ptr;
handle->_msg_ptr += rr->rdlength;
} else {
rr->ttl = 0;
rr->rdlength = 0;
rr->rdata = NULL;
}
handle->_rrnum++;
if (handle->_rrnum > handle->_counts[section]) {
handle->_sect = section + 1;
if (handle->_sect == ns_s_max) {
handle->_rrnum = -1;
handle->_msg_ptr = NULL;
} else {
handle->_rrnum = 0;
}
}
return 0;
bad:
errno = ENODEV;
return -1;
size:
errno = EMSGSIZE;
return -1;
}
int ns_name_uncompress(const unsigned char *msg, const unsigned char *eom,
const unsigned char *src, char *dst, size_t dstsiz)
{
int r;
r = dn_expand(msg, eom, src, dst, dstsiz);
if (r < 0) errno = EMSGSIZE;
return r;
}
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Merging ns_parse from Alpine
2014-12-14 0:43 Merging ns_parse from Alpine Rich Felker
@ 2014-12-14 7:38 ` Felix Janda
2014-12-14 17:23 ` Rich Felker
0 siblings, 1 reply; 5+ messages in thread
From: Felix Janda @ 2014-12-14 7:38 UTC (permalink / raw)
To: musl
Rich Felker wrote:
> I'm working on merging Timo's patch for ns_parse:
>
> http://git.alpinelinux.org/cgit/aports/tree/main/musl/1001-add-basic-dns-record-parsing-functions.patch?id=81d50064c335467fdfd80368bac6707d70db1af7
>
> The first issue that came up in the process is that arpa/nameser.h,
> which was previously not used by musl itself and really should never
> have been accepted in its current form, is full of junk like
> statement-expressions. Including it in a file that will be compiled
> with musl adds build dependency on these nonstandard features. I
> cleaned that up with no problem (just un-inlining the macros since
> we're adding function versions anyway), but there are a few more
> issues.
The NS_GET* macros still seem to be used a lot in the code.
> The main issue is that the parser functions have pointer arithmetic
> overflows (UB) checking against the end-of-message pointer. I've tried
> to fix that and I'm attaching a patch for review, along with my
> version of the fixed file. I'd appreciate comments on whether I missed
> anything.
>
> Other changes were mostly cosmetic or at least mechanical.
>
> Rich
I didn't notice any missed checks but I think that some checks can be
simplified:
[..]
> int ns_initparse(const unsigned char *msg, int msglen, ns_msg *handle)
> {
> int i, r;
>
> handle->_msg = msg;
> handle->_eom = msg + msglen;
> if (msglen < (2 + ns_s_max) * NS_INT16SZ) goto bad;
> NS_GET16(handle->_id, msg);
> NS_GET16(handle->_flags, msg);
> for (i = 0; i < ns_s_max; i++) {
> if (NS_INT16SZ > handle->_eom - msg) goto bad;
Isn't this uneccessary given the above check?
> NS_GET16(handle->_counts[i], msg);
> }
> for (i = 0; i < ns_s_max; i++) {
> if (handle->_counts[i]) {
> handle->_sections[i] = msg;
> r = ns_skiprr(msg, handle->_eom, i, handle->_counts[i]);
> if (r < 0) return -1;
> msg += r;
> } else {
> handle->_sections[i] = NULL;
> }
> }
> if (msg != handle->_eom) goto bad;
> handle->_sect = ns_s_max;
> handle->_rrnum = -1;
> handle->_msg_ptr = NULL;
> return 0;
> bad:
> errno = EMSGSIZE;
> return -1;
> }
>
> int ns_skiprr(const unsigned char *ptr, const unsigned char *eom, ns_sect section, int count)
> {
> const unsigned char *p = ptr;
> int r;
>
> while (count--) {
> r = dn_skipname(p, eom);
> if (r < 0) goto bad;
> if (r + 2 * NS_INT16SZ > eom - p) goto bad;
> p += r + 2 * NS_INT16SZ;
> if (section != ns_s_qd) {
> if (NS_INT32SZ + NS_INT16SZ > eom - p) goto bad;
> p += NS_INT32SZ;
> NS_GET16(r, p);
> if (r > eom - p) goto bad;
Couldn't the two checks be combined into one?
> p += r;
> }
> }
> return ptr - p;
> bad:
> errno = EMSGSIZE;
> return -1;
> }
>
> int ns_parserr(ns_msg *handle, ns_sect section, int rrnum, ns_rr *rr)
> {
> int r;
>
> if (section < 0 || section >= ns_s_max) goto bad;
> if (section != handle->_sect) {
> handle->_sect = section;
> handle->_rrnum = 0;
> handle->_msg_ptr = handle->_sections[section];
> }
> if (rrnum == -1) rrnum = handle->_rrnum;
> if (rrnum < 0 || rrnum >= handle->_counts[section]) goto bad;
> if (rrnum < handle->_rrnum) {
> handle->_rrnum = 0;
> handle->_msg_ptr = handle->_sections[section];
> }
> if (rrnum > handle->_rrnum) {
> r = ns_skiprr(handle->_msg_ptr, handle->_eom, section, rrnum - handle->_rrnum);
> if (r < 0) return -1;
> handle->_msg_ptr += r;
> handle->_rrnum = rrnum;
> }
> r = dn_expand(handle->_msg, handle->_eom, handle->_msg_ptr, rr->name, NS_MAXDNAME);
> if (r < 0) return -1;
dn_expand doesn't set errno.
> handle->_msg_ptr += r;
> if (2 * NS_INT16SZ > handle->_eom - handle->_msg_ptr) goto size;
> NS_GET16(rr->type, handle->_msg_ptr);
> NS_GET16(rr->rr_class, handle->_msg_ptr);
> if (section != ns_s_qd) {
> if (NS_INT32SZ + NS_INT16SZ > handle->_eom - handle->_msg_ptr) goto size;
> NS_GET32(rr->ttl, handle->_msg_ptr);
> NS_GET16(rr->rdlength, handle->_msg_ptr);
> if (rr->rdlength > handle->_eom - handle->_msg_ptr) goto size;
> rr->rdata = handle->_msg_ptr;
> handle->_msg_ptr += rr->rdlength;
> } else {
> rr->ttl = 0;
> rr->rdlength = 0;
> rr->rdata = NULL;
> }
> handle->_rrnum++;
> if (handle->_rrnum > handle->_counts[section]) {
> handle->_sect = section + 1;
> if (handle->_sect == ns_s_max) {
> handle->_rrnum = -1;
> handle->_msg_ptr = NULL;
> } else {
> handle->_rrnum = 0;
> }
> }
> return 0;
> bad:
> errno = ENODEV;
> return -1;
> size:
> errno = EMSGSIZE;
> return -1;
> }
>
> int ns_name_uncompress(const unsigned char *msg, const unsigned char *eom,
> const unsigned char *src, char *dst, size_t dstsiz)
> {
> int r;
> r = dn_expand(msg, eom, src, dst, dstsiz);
> if (r < 0) errno = EMSGSIZE;
> return r;
> }
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Merging ns_parse from Alpine
2014-12-14 7:38 ` Felix Janda
@ 2014-12-14 17:23 ` Rich Felker
2014-12-14 19:05 ` Felix Janda
0 siblings, 1 reply; 5+ messages in thread
From: Rich Felker @ 2014-12-14 17:23 UTC (permalink / raw)
To: musl
On Sun, Dec 14, 2014 at 08:38:15AM +0100, Felix Janda wrote:
> Rich Felker wrote:
> > I'm working on merging Timo's patch for ns_parse:
> >
> > http://git.alpinelinux.org/cgit/aports/tree/main/musl/1001-add-basic-dns-record-parsing-functions.patch?id=81d50064c335467fdfd80368bac6707d70db1af7
> >
> > The first issue that came up in the process is that arpa/nameser.h,
> > which was previously not used by musl itself and really should never
> > have been accepted in its current form, is full of junk like
> > statement-expressions. Including it in a file that will be compiled
> > with musl adds build dependency on these nonstandard features. I
> > cleaned that up with no problem (just un-inlining the macros since
> > we're adding function versions anyway), but there are a few more
> > issues.
>
> The NS_GET* macros still seem to be used a lot in the code.
Yes because they also advance the pointer, and this behavior was
intentional in the code. I don't think it hurts to use them once
they're fixed to be function calls.
> I didn't notice any missed checks but I think that some checks can be
> simplified:
>
> [..]
> > int ns_initparse(const unsigned char *msg, int msglen, ns_msg *handle)
> > {
> > int i, r;
> >
> > handle->_msg = msg;
> > handle->_eom = msg + msglen;
> > if (msglen < (2 + ns_s_max) * NS_INT16SZ) goto bad;
>
> > NS_GET16(handle->_id, msg);
> > NS_GET16(handle->_flags, msg);
> > for (i = 0; i < ns_s_max; i++) {
> > if (NS_INT16SZ > handle->_eom - msg) goto bad;
>
> Isn't this uneccessary given the above check?
I think you're right. I missed that.
> [...]
> > int ns_skiprr(const unsigned char *ptr, const unsigned char *eom, ns_sect section, int count)
> > {
> > const unsigned char *p = ptr;
> > int r;
> >
> > while (count--) {
> > r = dn_skipname(p, eom);
> > if (r < 0) goto bad;
> > if (r + 2 * NS_INT16SZ > eom - p) goto bad;
> > p += r + 2 * NS_INT16SZ;
> > if (section != ns_s_qd) {
> > if (NS_INT32SZ + NS_INT16SZ > eom - p) goto bad;
> > p += NS_INT32SZ;
> > NS_GET16(r, p);
> > if (r > eom - p) goto bad;
>
> Couldn't the two checks be combined into one?
No, r is not read until after the first check, using the result of the
first check. The read is hidden in the hideous macro that stores a
result rather than returning it...
> > int ns_parserr(ns_msg *handle, ns_sect section, int rrnum, ns_rr *rr)
> > {
> > int r;
> >
> > if (section < 0 || section >= ns_s_max) goto bad;
> > if (section != handle->_sect) {
> > handle->_sect = section;
> > handle->_rrnum = 0;
> > handle->_msg_ptr = handle->_sections[section];
> > }
> > if (rrnum == -1) rrnum = handle->_rrnum;
> > if (rrnum < 0 || rrnum >= handle->_counts[section]) goto bad;
> > if (rrnum < handle->_rrnum) {
> > handle->_rrnum = 0;
> > handle->_msg_ptr = handle->_sections[section];
> > }
> > if (rrnum > handle->_rrnum) {
> > r = ns_skiprr(handle->_msg_ptr, handle->_eom, section, rrnum - handle->_rrnum);
> > if (r < 0) return -1;
> > handle->_msg_ptr += r;
> > handle->_rrnum = rrnum;
> > }
> > r = dn_expand(handle->_msg, handle->_eom, handle->_msg_ptr, rr->name, NS_MAXDNAME);
> > if (r < 0) return -1;
>
> dn_expand doesn't set errno.
Maybe we should just call ns_name_uncompress (below) instead here?
> [...]
> > int ns_name_uncompress(const unsigned char *msg, const unsigned char *eom,
> > const unsigned char *src, char *dst, size_t dstsiz)
> > {
> > int r;
> > r = dn_expand(msg, eom, src, dst, dstsiz);
> > if (r < 0) errno = EMSGSIZE;
> > return r;
> > }
Does that sound better?
Rich
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Merging ns_parse from Alpine
2014-12-14 17:23 ` Rich Felker
@ 2014-12-14 19:05 ` Felix Janda
2014-12-14 22:56 ` Rich Felker
0 siblings, 1 reply; 5+ messages in thread
From: Felix Janda @ 2014-12-14 19:05 UTC (permalink / raw)
To: musl
Rich Felker wrote:
> On Sun, Dec 14, 2014 at 08:38:15AM +0100, Felix Janda wrote:
> > Rich Felker wrote:
> > > I'm working on merging Timo's patch for ns_parse:
> > >
> > > http://git.alpinelinux.org/cgit/aports/tree/main/musl/1001-add-basic-dns-record-parsing-functions.patch?id=81d50064c335467fdfd80368bac6707d70db1af7
> > >
> > > The first issue that came up in the process is that arpa/nameser.h,
> > > which was previously not used by musl itself and really should never
> > > have been accepted in its current form, is full of junk like
> > > statement-expressions. Including it in a file that will be compiled
> > > with musl adds build dependency on these nonstandard features. I
> > > cleaned that up with no problem (just un-inlining the macros since
> > > we're adding function versions anyway), but there are a few more
> > > issues.
> >
> > The NS_GET* macros still seem to be used a lot in the code.
>
> Yes because they also advance the pointer, and this behavior was
> intentional in the code. I don't think it hurts to use them once
> they're fixed to be function calls.
I likely just missunderstood you. With un-inlining the macros you mean
making them call the ns_get* functions? (That seems reasonable.)
I agree with your comments on ns_initparse and ns_skiprr.
[...]
> > > int ns_parserr(ns_msg *handle, ns_sect section, int rrnum, ns_rr *rr)
> > > {
> > > int r;
> > >
> > > if (section < 0 || section >= ns_s_max) goto bad;
> > > if (section != handle->_sect) {
> > > handle->_sect = section;
> > > handle->_rrnum = 0;
> > > handle->_msg_ptr = handle->_sections[section];
> > > }
> > > if (rrnum == -1) rrnum = handle->_rrnum;
> > > if (rrnum < 0 || rrnum >= handle->_counts[section]) goto bad;
> > > if (rrnum < handle->_rrnum) {
> > > handle->_rrnum = 0;
> > > handle->_msg_ptr = handle->_sections[section];
> > > }
> > > if (rrnum > handle->_rrnum) {
> > > r = ns_skiprr(handle->_msg_ptr, handle->_eom, section, rrnum - handle->_rrnum);
> > > if (r < 0) return -1;
> > > handle->_msg_ptr += r;
> > > handle->_rrnum = rrnum;
> > > }
> > > r = dn_expand(handle->_msg, handle->_eom, handle->_msg_ptr, rr->name, NS_MAXDNAME);
> > > if (r < 0) return -1;
> >
> > dn_expand doesn't set errno.
>
> Maybe we should just call ns_name_uncompress (below) instead here?
>
> > [...]
> > > int ns_name_uncompress(const unsigned char *msg, const unsigned char *eom,
> > > const unsigned char *src, char *dst, size_t dstsiz)
> > > {
> > > int r;
> > > r = dn_expand(msg, eom, src, dst, dstsiz);
> > > if (r < 0) errno = EMSGSIZE;
> > > return r;
> > > }
>
> Does that sound better?
That should work. I have no particular opinion on whether to call
ns_name_uncompress or to jump to "size".
Felix
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Merging ns_parse from Alpine
2014-12-14 19:05 ` Felix Janda
@ 2014-12-14 22:56 ` Rich Felker
0 siblings, 0 replies; 5+ messages in thread
From: Rich Felker @ 2014-12-14 22:56 UTC (permalink / raw)
To: musl
On Sun, Dec 14, 2014 at 08:05:36PM +0100, Felix Janda wrote:
> Rich Felker wrote:
> > On Sun, Dec 14, 2014 at 08:38:15AM +0100, Felix Janda wrote:
> > > Rich Felker wrote:
> > > > I'm working on merging Timo's patch for ns_parse:
> > > >
> > > > http://git.alpinelinux.org/cgit/aports/tree/main/musl/1001-add-basic-dns-record-parsing-functions.patch?id=81d50064c335467fdfd80368bac6707d70db1af7
> > > >
> > > > The first issue that came up in the process is that arpa/nameser.h,
> > > > which was previously not used by musl itself and really should never
> > > > have been accepted in its current form, is full of junk like
> > > > statement-expressions. Including it in a file that will be compiled
> > > > with musl adds build dependency on these nonstandard features. I
> > > > cleaned that up with no problem (just un-inlining the macros since
> > > > we're adding function versions anyway), but there are a few more
> > > > issues.
> > >
> > > The NS_GET* macros still seem to be used a lot in the code.
> >
> > Yes because they also advance the pointer, and this behavior was
> > intentional in the code. I don't think it hurts to use them once
> > they're fixed to be function calls.
>
> I likely just missunderstood you. With un-inlining the macros you mean
> making them call the ns_get* functions? (That seems reasonable.)
Right:
#define NS_GET16(s, cp) (void)((s) = ns_get16(((cp)+=2)-2))
#define NS_GET32(l, cp) (void)((l) = ns_get32(((cp)+=4)-4))
#define NS_PUT16(s, cp) ns_put16((s), ((cp)+=2)-2)
#define NS_PUT32(l, cp) ns_put32((l), ((cp)+=4)-4)
> > > > r = dn_expand(handle->_msg, handle->_eom, handle->_msg_ptr, rr->name, NS_MAXDNAME);
> > > > if (r < 0) return -1;
> > >
> > > dn_expand doesn't set errno.
> >
> > Maybe we should just call ns_name_uncompress (below) instead here?
> >
> > > [...]
> > > > int ns_name_uncompress(const unsigned char *msg, const unsigned char *eom,
> > > > const unsigned char *src, char *dst, size_t dstsiz)
> > > > {
> > > > int r;
> > > > r = dn_expand(msg, eom, src, dst, dstsiz);
> > > > if (r < 0) errno = EMSGSIZE;
> > > > return r;
> > > > }
> >
> > Does that sound better?
>
> That should work. I have no particular opinion on whether to call
> ns_name_uncompress or to jump to "size".
Indeed, ns_name_uncompress doesn't set any errors except EMSGSIZE. The
only ways dn_expand can fail are truncated buffers or malformed
messages that would attempt to read outside the buffer, and I think
EMSGSIZE is reasonable for either of these.
Rich
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-12-14 22:56 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-14 0:43 Merging ns_parse from Alpine Rich Felker
2014-12-14 7:38 ` Felix Janda
2014-12-14 17:23 ` Rich Felker
2014-12-14 19:05 ` Felix Janda
2014-12-14 22:56 ` Rich Felker
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/musl/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).