[PATCH] [BZ #20900] Call __res_vinit if _PATH_RESCONF is changed

[PATCH] [BZ #20900] Call __res_vinit if _PATH_RESCONF is changed

[H.J. Lu]

The content of _PATH_RESCONF may change over time with DHCP.
__res_maybe_init should check if _PATH_RESCONF is changed.  Otherwise
a long-running process will get wrong DNS results if _PATH_RESCONF is
changed by DHCP.

Any comments?

H.J.
---
	[BZ #20900]
	* resolv/res_libc.c: Include <sys/stat.h>.
	(__res_maybe_init): Call __res_vinit if _PATH_RESCONF has been
	changed since the last time.
---
 resolv/res_libc.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/resolv/res_libc.c b/resolv/res_libc.c
index a4b376f..47b2d42 100644
--- a/resolv/res_libc.c
+++ b/resolv/res_libc.c
@@ -23,6 +23,7 @@
 #include <sys/types.h>
 #include <netinet/in.h>
 #include <arpa/nameser.h>
+#include <sys/stat.h>
 #include <resolv.h>
 #include <libc-lock.h>
 
@@ -97,6 +98,21 @@ __res_maybe_init (res_state resp, int preinit)
 			if (resp->nscount > 0)
 				__res_iclose (resp, true);
 			return __res_vinit (resp, 1);
+		} else {
+			struct stat buf;
+
+			/* Call __res_vinit if _PATH_RESCONF has been
+			   changed since the last time.  */
+			if (stat (_PATH_RESCONF, &buf) == 0) {
+				static struct timespec mtime;
+				if (mtime.tv_sec != buf.st_mtim.tv_sec
+				    || mtime.tv_nsec != buf.st_mtim.tv_nsec) {
+					mtime = buf.st_mtim;
+					if (resp->nscount > 0)
+						__res_iclose (resp, true);
+					return __res_vinit (resp, 1);
+				}
+			}
 		}
 		return 0;
 	} else if (preinit) {
-- 
2.7.4

Re: [PATCH] [BZ #20900] Call __res_vinit if _PATH_RESCONF is changed

[Joseph Myers]

On Thu, 1 Dec 2016, H.J. Lu wrote:

> The content of _PATH_RESCONF may change over time with DHCP.
> __res_maybe_init should check if _PATH_RESCONF is changed.  Otherwise
> a long-running process will get wrong DNS results if _PATH_RESCONF is
> changed by DHCP.
> 
> Any comments?

This bug is either a duplicate of or related to bug 984.  There are 
various patches out there, e.g. one in Debian, and also claims of problems 
with those patches.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] [BZ #20900] Call __res_vinit if _PATH_RESCONF is changed

[Andreas Schwab]

On Dez 01 2016, "H.J. Lu" <hongjiu.lu@intel.com> wrote:

> @@ -97,6 +98,21 @@ __res_maybe_init (res_state resp, int preinit)
>  			if (resp->nscount > 0)
>  				__res_iclose (resp, true);
>  			return __res_vinit (resp, 1);
> +		} else {
> +			struct stat buf;
> +
> +			/* Call __res_vinit if _PATH_RESCONF has been
> +			   changed since the last time.  */
> +			if (stat (_PATH_RESCONF, &buf) == 0) {
> +				static struct timespec mtime;
> +				if (mtime.tv_sec != buf.st_mtim.tv_sec
> +				    || mtime.tv_nsec != buf.st_mtim.tv_nsec) {
> +					mtime = buf.st_mtim;
> +					if (resp->nscount > 0)
> +						__res_iclose (resp, true);
> +					return __res_vinit (resp, 1);

This isn't thread-safe.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] [BZ #20900] Call __res_vinit if _PATH_RESCONF is changed

[H.J. Lu]

On Thu, Dec 1, 2016 at 11:02 AM, Joseph Myers <joseph@codesourcery.com> wrote:
> On Thu, 1 Dec 2016, H.J. Lu wrote:
>
>> The content of _PATH_RESCONF may change over time with DHCP.
>> __res_maybe_init should check if _PATH_RESCONF is changed.  Otherwise
>> a long-running process will get wrong DNS results if _PATH_RESCONF is
>> changed by DHCP.
>>
>> Any comments?
>
> This bug is either a duplicate of or related to bug 984.  There are
> various patches out there, e.g. one in Debian, and also claims of problems
> with those patches.

It looks very similar.  Does anyone have a link to Debian's patch?


-- 
H.J.

Re: [PATCH] [BZ #20900] Call __res_vinit if _PATH_RESCONF is changed

[H.J. Lu]

On Thu, Dec 1, 2016 at 11:21 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
> On Dez 01 2016, "H.J. Lu" <hongjiu.lu@intel.com> wrote:
>
>> @@ -97,6 +98,21 @@ __res_maybe_init (res_state resp, int preinit)
>>                       if (resp->nscount > 0)
>>                               __res_iclose (resp, true);
>>                       return __res_vinit (resp, 1);
>> +             } else {
>> +                     struct stat buf;
>> +
>> +                     /* Call __res_vinit if _PATH_RESCONF has been
>> +                        changed since the last time.  */
>> +                     if (stat (_PATH_RESCONF, &buf) == 0) {
>> +                             static struct timespec mtime;
>> +                             if (mtime.tv_sec != buf.st_mtim.tv_sec
>> +                                 || mtime.tv_nsec != buf.st_mtim.tv_nsec) {
>> +                                     mtime = buf.st_mtim;
>> +                                     if (resp->nscount > 0)
>> +                                             __res_iclose (resp, true);
>> +                                     return __res_vinit (resp, 1);
>
> This isn't thread-safe.
>

True.  We can use __thread:

diff --git a/resolv/res_libc.c b/resolv/res_libc.c
index 47b2d42..f5b40ae 100644
--- a/resolv/res_libc.c
+++ b/resolv/res_libc.c
@@ -104,7 +104,7 @@ __res_maybe_init (res_state resp, int preinit)
         /* Call __res_vinit if _PATH_RESCONF has been
            changed since the last time.  */
         if (stat (_PATH_RESCONF, &buf) == 0) {
-           static struct timespec mtime;
+           static __thread struct timespec mtime;
            if (mtime.tv_sec != buf.st_mtim.tv_sec
                || mtime.tv_nsec != buf.st_mtim.tv_nsec) {
               mtime = buf.st_mtim;


-- 
H.J.

Re: [PATCH] [BZ #20900] Call __res_vinit if _PATH_RESCONF is changed

[Florian Weimer]

On 12/01/2016 07:56 PM, H.J. Lu wrote:
> The content of _PATH_RESCONF may change over time with DHCP.
> __res_maybe_init should check if _PATH_RESCONF is changed.  Otherwise
> a long-running process will get wrong DNS results if _PATH_RESCONF is
> changed by DHCP.
>
> Any comments?

This is not the correct approach.  You need a pristine copy of _res and 
check if the application hasn't made any changes because this is a 
public interface and such changes are expected (and in one case, even 
encouraged).  The existing behavior (re-init all threads if we re-init 
one) is buggy for the same reason.

I'm working on a new resolv.conf parser which will address these issues 
and also eliminate the search path limit.

Florian

Re: [PATCH] [BZ #20900] Call __res_vinit if _PATH_RESCONF is changed

[Mike Frysinger]

On Thu, Dec 1, 2016 at 2:23 PM, H.J. Lu wrote:
> On Thu, Dec 1, 2016 at 11:02 AM, Joseph Myers wrote:
>> On Thu, 1 Dec 2016, H.J. Lu wrote:
>>
>>> The content of _PATH_RESCONF may change over time with DHCP.
>>> __res_maybe_init should check if _PATH_RESCONF is changed.  Otherwise
>>> a long-running process will get wrong DNS results if _PATH_RESCONF is
>>> changed by DHCP.
>>>
>>> Any comments?
>>
>> This bug is either a duplicate of or related to bug 984.  There are
>> various patches out there, e.g. one in Debian, and also claims of problems
>> with those patches.
>
> It looks very similar.  Does anyone have a link to Debian's patch?

here's the patch i've been carrying in Gentoo for almost a decade, and
i took it from SUSE:
https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=727cea2ca77b47681f4678d5fb3180aab89fe9ab
-mike

Re: [PATCH] [BZ #20900] Call __res_vinit if _PATH_RESCONF is changed

[Andreas Schwab]

On Dez 02 2016, Mike Frysinger <vapier@gentoo.org> wrote:

> i took it from SUSE:
> https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=727cea2ca77b47681f4678d5fb3180aab89fe9ab

This is outdated, here is the current version used by openSUSE:

https://build.opensuse.org/package/view_file/openSUSE:Factory/glibc/glibc-resolv-reload.diff?expand=1

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] [BZ #20900] Call __res_vinit if _PATH_RESCONF is changed

[Mike Frysinger]

On Fri, Dec 2, 2016 at 8:48 AM, Andreas Schwab wrote:
> On Dez 02 2016, Mike Frysinger wrote:
>> i took it from SUSE:
>> https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=727cea2ca77b47681f4678d5fb3180aab89fe9ab
>
> This is outdated, here is the current version used by openSUSE:
>
> https://build.opensuse.org/package/view_file/openSUSE:Factory/glibc/glibc-resolv-reload.diff?expand=1

is the new lock there to deal with possible non-atomic updates to
resconf_mtime ?  e.g. a 64-bit time_t on a system w/32-bit registers.
-mike