[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20141214172342.GG4574@brightrain.aerifal.cx>
Date: Sun, 14 Dec 2014 12:23:42 -0500
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: Merging ns_parse from Alpine
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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
