From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/11303 Path: news.gmane.org!.POSTED!not-for-mail From: Stefan Sedich Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH v3] Add RES_OPTIONS support for resolv.conf options overriding Date: Tue, 25 Apr 2017 20:56:39 +0000 Message-ID: References: <20170425194557.8520-1-stefan.sedich@gmail.com> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/alternative; boundary=001a113c15b47d6cfc054e03f659 X-Trace: blaine.gmane.org 1493153827 2260 195.159.176.226 (25 Apr 2017 20:57:07 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Tue, 25 Apr 2017 20:57:07 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-11318-gllmg-musl=m.gmane.org@lists.openwall.com Tue Apr 25 22:57:03 2017 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.84_2) (envelope-from ) id 1d37W6-0000MU-QS for gllmg-musl@m.gmane.org; Tue, 25 Apr 2017 22:56:58 +0200 Original-Received: (qmail 30555 invoked by uid 550); 25 Apr 2017 20:57:03 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 30528 invoked from network); 25 Apr 2017 20:57:02 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to; bh=LyjgN+Zkkao9FCTFiuREHxp0bnnNpYPgCXzAhWPuWwc=; b=oiu0r1Soqfun+ApGgDd7vOmXB/c/AtO4t7gK/SwhCRMsV0PUOARvHt+eOzZShRXFBg VXfgHAkvAiZrA9bWCp/SoTdW+Jzke2h5FPpQq4TULAiO9hBlS1bYO/TA4l7I9zF5UOsy HN2hT0NnsQcUk5+DOr/bnX8G5WYJ8Bl/6Okc/Q9KjtnDTNGcbrH5G59z64UNTB+8ia1G A5/Z1pRF6ypMMTawTZqveKnYGTSxwawmvjD6XZ7QLt5Z3Z0uo1leQbYLvuYy9dVeBMOd MJ9/Sjs7Oy0W8DeAduo5SBXkzuDBNLvPf/8tuBzL4juH2Q1tFxNlM+Cce1LKyPAriXjv D4dQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=LyjgN+Zkkao9FCTFiuREHxp0bnnNpYPgCXzAhWPuWwc=; b=a0QLncnz0DiCz96PDWYaSTj+TcXHpXwQlCBfbq08RCgW0u40VtHAJrAPeNtvHiebjy PG5gMnXns++dAM5rHrJvN/kc8X9e+L1d5N3wu5JrTgCo929r0mP97O3MtBo43WOVAieS rk8PW3L6urjtNRXOMtuIHcPBliNbkQzERCiYmmSefe4mBTXURhj+Ni4qoRir4FCiJcrm x56A/LE6jLjL+kGNkJRTlFFqi/R7h49tCdNjaMddNVY6gCJIVrRJh9ZTNn75W5IApiLc +GQ6sd6rEUW7yfx2CfO0NRrxsu6XEq7AnVeNC6Ddy10J3ujeME2kEXvKd38VUJOZMz4B cmnA== X-Gm-Message-State: AN3rC/4FBR87hCY4aEb86Zm+DItyrxn2zkLsBcfDtrq7wecwXImO7oJA tu+5JfsqsT+Zo1PbwOrgiUPKbWSMqKkL X-Received: by 10.25.181.23 with SMTP id e23mr10647407lff.98.1493153810834; Tue, 25 Apr 2017 13:56:50 -0700 (PDT) In-Reply-To: Xref: news.gmane.org gmane.linux.lib.musl.general:11303 Archived-At: --001a113c15b47d6cfc054e03f659 Content-Type: text/plain; charset=UTF-8 On Tue, Apr 25, 2017 at 1:50 PM Alexander Monakov wrote: > I see a couple of pre-existing issues in options parsing > (not your problem, just questions for Rich): > > On Tue, 25 Apr 2017, Stefan Sedich wrote: > > +void __parse_resolv_opts(struct resolvconf *conf, char *opts) > > +{ > > + char *p, *z; > > + > > + p = strstr(opts, "ndots:"); > > This accepts xndots, _ndots, etc. I think this is undesirable, prefixing a > character could be seen by some users as a way to "comment-out" an option > without deleting it, and such loose matching lays a trap for them. It also > breaks if a valid option ending in 'ndots' appears in the future. > > > + p = strstr(opts, "timeout:"); > > + if (p && (isdigit(p[8]) || p[8]=='.')) { > > + p += 8; > > + unsigned long x = strtoul(p, &z, 10); > > Either stroul should be strtod, or accepting p[8]=='.' is pointless. > This was introduced in commit d6cb08bcaca4ff1f921375510ca72bccea969c75 > that moved this chunk of code from res_msend.c to resolvconf.c and > introduced p[8]=='.' check en passant. > > > @@ -89,5 +96,11 @@ no_resolv_conf: > > > > conf->nns = nns; > > > > + char *res_opts_env = NULL; > > + if (!libc.secure) res_opts_env = getenv("RES_OPTIONS"); > > + if (res_opts_env) { > > + __parse_resolv_opts(conf, res_opts_env); > > + } > > This might look slightly cleaner if written as > > if (!libc.secure) { > const char *opts = getenv("RES_OPTIONS"); > if (opts) __parse_resolve_opts(conf, opts); > } > > Alexander > Alexander, I agree on the cleaner version :), I will await Rich to comment on the rest before opening another patch as perhaps I can address some of these as part of this commit if desired? --001a113c15b47d6cfc054e03f659 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On Tue= , Apr 25, 2017 at 1:50 PM Alexander Monakov <amonakov@ispras.ru> wrote:
I see a couple of pre-existing issues in options parsing
(not your problem, just questions for Rich):

On Tue, 25 Apr 2017, Stefan Sedich wrote:
> +void __parse_resolv_opts(struct resolvconf *conf, char *opts)
> +{
> +=C2=A0 =C2=A0 =C2=A0char *p, *z;
> +
> +=C2=A0 =C2=A0 =C2=A0p =3D strstr(opts, "ndots:");

This accepts xndots, _ndots, etc.=C2=A0 I think this is undesirable, prefix= ing a
character could be seen by some users as a way to "comment-out" a= n option
without deleting it, and such loose matching lays a trap for them. It also<= br> breaks if a valid option ending in 'ndots' appears in the future.
> +=C2=A0 =C2=A0 =C2=A0p =3D strstr(opts, "timeout:");
> +=C2=A0 =C2=A0 =C2=A0if (p && (isdigit(p[8]) || p[8]=3D=3D'= ;.')) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0p +=3D 8;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0unsigned long x =3D s= trtoul(p, &z, 10);

Either stroul should be strtod, or accepting p[8]=3D=3D'.' is point= less.
This was introduced in commit d6cb08bcaca4ff1f921375510ca72bccea969c75
that moved this chunk of code from res_msend.c to resolvconf.c and
introduced p[8]=3D=3D'.' check en passant.

> @@ -89,5 +96,11 @@ no_resolv_conf:
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0conf->nns =3D nns;
>
> +=C2=A0 =C2=A0 =C2=A0char *res_opts_env =3D NULL;
> +=C2=A0 =C2=A0 =C2=A0if (!libc.secure) res_opts_env =3D getenv("R= ES_OPTIONS");
> +=C2=A0 =C2=A0 =C2=A0if (res_opts_env) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0__parse_resolv_opts(c= onf, res_opts_env);
> +=C2=A0 =C2=A0 =C2=A0}

This might look slightly cleaner if written as

=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!libc.secure) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 const char *opts = =3D getenv("RES_OPTIONS");
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (opts) __parse_r= esolve_opts(conf, opts);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 }

Alexander

Alexander,

I agree on the cleaner version :), I will await Rich to comment on t= he rest before opening another patch as perhaps I can address some of these= as part of this commit if desired?=C2=A0
--001a113c15b47d6cfc054e03f659--