Last Comment Bug 810716 - HAVE_RES_NINIT is eroneously set on NetBSD
: HAVE_RES_NINIT is eroneously set on NetBSD
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: 16 Branch
: x86_64 NetBSD
: -- normal (vote)
: mozilla28
Assigned To: Martin Husemann
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-11 14:23 PST by Martin Husemann
Modified: 2013-11-19 10:30 PST (History)
5 users (show)
Ms2ger: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
avoid usage of the _res global resolver state in threaded programs on NetBSD (402 bytes, patch)
2012-11-12 02:33 PST, Martin Husemann
no flags Details | Diff | Splinter Review
Bug 810716 - Make res_ninit() check work on BSDs. (2.82 KB, patch)
2012-11-12 07:36 PST, Jan Beich
no flags Details | Diff | Splinter Review
Bug 810716 - Make res_ninit() check work on BSDs. (2.87 KB, patch)
2012-11-21 15:57 PST, Jan Beich
ted: review-
Details | Diff | Splinter Review
consistent includes (2.00 KB, patch)
2013-09-21 02:05 PDT, Jan Beich
ted: review+
Details | Diff | Splinter Review
exclude broken (1.40 KB, patch)
2013-09-21 02:20 PDT, Jan Beich
ted: review+
Details | Diff | Splinter Review
test program (714 bytes, text/x-csrc)
2013-09-21 02:25 PDT, Jan Beich
no flags Details
Avoid AC_TEST_LINK on systems where we do not want res_ninit (2.30 KB, patch)
2013-11-01 06:38 PDT, Martin Husemann
ted: review+
Details | Diff | Splinter Review
consistent includes (2.25 KB, patch)
2013-11-16 04:11 PST, Jan Beich
ted: review+
Details | Diff | Splinter Review

Description Martin Husemann 2012-11-11 14:23:48 PST
NetBSD has a res_ninit() function and a _res global resolver state, but the latter is only available in non threaded programs.

However, the naive configure test does not use -pthread for the link test nor does it run a test program (which would abort at runtime), so it does define HAVE_RES_NINIT while it better should not. This makes Firefox abort at runtime.

My autoconfig fu is extremely weak and I'm not even sure what the right solution would be. The thread safe versions of the resolver functions should be used instead anyway?
Comment 1 Martin Husemann 2012-11-12 02:33:28 PST
Created attachment 680567 [details] [diff] [review]
avoid usage of the _res global resolver state in threaded programs on NetBSD
Comment 2 Jan Beich 2012-11-12 07:36:04 PST
Created attachment 680629 [details] [diff] [review]
Bug 810716 - Make res_ninit() check work on BSDs.

Martin, can you confirm AC_TRY_RUN works for you? -pthread should be already in LDFLAGS, added earlier by USE_PTHREADS check.

Other changes workaround resolv.h being not self-contained on some systems. This is already done in https://mxr.mozilla.org/mozilla-central/source/netwerk/dns/nsHostResolver.cpp

  /usr/include/resolv.h:157:14: error: array has incomplete element type
	'struct sockaddr_in'
		  nsaddr_list[MAXNS];     /*%< address of name server */
			     ^
  /usr/include/resolv.h:156:9: note: forward declaration of 'struct sockaddr_in'
	  struct sockaddr_in
		 ^
  /usr/include/resolv.h:171:18: error: field has incomplete type 'struct in_addr'
		  struct in_addr  addr;
				  ^
  /usr/include/resolv.h:171:10: note: forward declaration of 'struct in_addr'
		  struct in_addr  addr;
			 ^
  /usr/include/resolv.h:195:21: error: field has incomplete type 'struct sockaddr_in'
	  struct sockaddr_in      sin;
				  ^
  /usr/include/resolv.h:156:9: note: forward declaration of 'struct sockaddr_in'
	  struct sockaddr_in
		 ^
  3 errors generated.
Comment 3 Martin Husemann 2012-11-12 09:22:00 PST
This seems to work (or better: fail as expected):

configure:12259: checking for res_ninit()
configure:12281: gcc -o conftest -O2 -pipe -I/usr/pkg/include -I/usr/include -I/usr/pkg/include/nspr -I/usr/X11R7/include -I/usr/X11R7/include/freetype2 -fno-strict-aliasing -Dunix -ffunction-sections -fdata-sections -pthread -I/usr/pkg/include -I/usr/include -I/usr/pkg/include/nspr -I/usr/X11R7/include -I/usr/X11R7/include/freetype2 -lpthread -Wl,-rpath,/usr/pkg/lib/xulrunner -Wl,-rpath,/usr/pkg/lib -L/usr/pkg/lib -L/usr/lib -Wl,-R/usr/lib -Wl,-R/usr/pkg/lib -L/usr/pkg/lib/nspr -Wl,-R/usr/pkg/lib/nspr -L/usr/pkg/lib/nss -Wl,-R/usr/pkg/lib/nss -L/usr/X11R7/lib -Wl,-R/usr/X11R7/lib -Wl,-z,noexecstack conftest.c   1>&5
configure: failed program was:
#line 12267 "configure"
#include "confdefs.h"

        #ifdef linux
        #define _BSD_SOURCE 1
        #endif
        #include <sys/types.h>
        #include <netinet/in.h>
        #include <arpa/nameser.h>
        #include <resolv.h>
        int main(int argc, char **argv){
            int foo = res_ninit(&_res);
        }
Comment 4 Jan Beich 2012-11-21 15:57:14 PST
Created attachment 684218 [details] [diff] [review]
Bug 810716 - Make res_ninit() check work on BSDs.

forgot explicit 'return 0' (pre-C99)
Comment 5 Ted Mielczarek [:ted.mielczarek] 2012-12-05 16:47:12 PST
Comment on attachment 684218 [details] [diff] [review]
Bug 810716 - Make res_ninit() check work on BSDs.

Review of attachment 684218 [details] [diff] [review]:
-----------------------------------------------------------------

We don't like to use AC_TRY_RUN because it doesn't work in cross-compiles. If you really can't detect this in a sane manner via compile tests then it's okay to just check the target OS and hardcode the answer for BSD.
Comment 6 Martin Husemann 2012-12-06 00:47:33 PST
Out of curiosity: are there any OS where use of _res in threaded programs is not a bug?
If the surrounding code actually handles locking elsewhere (I have no clue), it would be easy to use a static local variable instead and avoid the libc instance.
Comment 7 Jan Beich 2013-09-21 02:05:57 PDT
Created attachment 808118 [details] [diff] [review]
consistent includes

FreeBSD and DragonFly lack <netinet/in.h> in <resolv.h> leading to an error:

/usr/include/resolv.h:157:14: error: array has incomplete element type
      'struct sockaddr_in'
                nsaddr_list[MAXNS];     /*%< address of name server */
                           ^
/usr/include/resolv.h:156:9: note: forward declaration of 'struct sockaddr_in'
        struct sockaddr_in
               ^
/usr/include/resolv.h:171:18: error: field has incomplete type 'struct in_addr'
                struct in_addr  addr;
                                ^
/usr/include/resolv.h:171:10: note: forward declaration of 'struct in_addr'
                struct in_addr  addr;
                       ^
/usr/include/resolv.h:195:21: error: field has incomplete type 'struct sockaddr_in'
        struct sockaddr_in      sin;
                                ^
/usr/include/resolv.h:156:9: note: forward declaration of 'struct sockaddr_in'
        struct sockaddr_in
               ^
3 errors generated.

Other systems recommend to include more than just resolv.h as well:
http://man7.org/linux/man-pages/man3/resolver.3.html
https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man3/res_init.3.html
http://docs.oracle.com/cd/E23823_01/html/816-5170/res-ninit-3resolv.html
http://publib.boulder.ibm.com/infocenter/iseries/v5r4/topic/apis/resninit.htm
http://www.qnx.com/developers/docs/6.5.0/topic/com.qnx.doc.neutrino_lib_ref/r/res_init.html
Comment 8 Jan Beich 2013-09-21 02:20:08 PDT
Created attachment 808122 [details] [diff] [review]
exclude broken
Comment 9 Jan Beich 2013-09-21 02:25:48 PDT
Created attachment 808130 [details]
test program

According to res_init(3) man page and my test program _res is thread-safe on FreeBSD and DragonFly.

freebsd   - bar: 1234 foo: 4512 main: 0
dragonfly - bar: 1234 foo: 34387 main: 0
openbsd   - bar: 1234 foo: 1234 main: 1234
netbsd    - abort trap

I'm not familiar enough with Linux to test there.
Comment 10 Jan Beich 2013-09-22 22:37:24 PDT
_res from -lbind seems usable on NetBSD (and only there)

libbind on freebsd   - bar: 1234 foo: 1234 main: 1234
libbind on dragonfly - bar: 1234 foo: 1234 main: 1234
libbind on openbsd   - undefined reference to `__res_state'
libbind on netbsd    - bar: 1234 foo: 21374 main: 60471
Comment 11 Martin Husemann 2013-09-23 00:54:25 PDT
(In reply to Jan Beich from comment #10)
> _res from -lbind seems usable on NetBSD (and only there)

Only in some NetBSD versions, and we can not rely on it as a future-proof path.
Comment 12 Ted Mielczarek [:ted.mielczarek] 2013-10-01 12:07:46 PDT
Comment on attachment 808122 [details] [diff] [review]
exclude broken

Review of attachment 808122 [details] [diff] [review]:
-----------------------------------------------------------------

Is there any reason you can't just do this test at the autoconf level, instead of stuffing it down into the source being tested? Is it more work that way?
Comment 13 Martin Husemann 2013-10-01 12:28:05 PDT
Yes, if we do not really test, the hard-coded list could just be in configure.in.
Comment 14 Martin Husemann 2013-10-01 12:38:28 PDT
Another point: now that Jan has answered the real question (the code assumes _res to be thread local), we can start fixing this correctly by providing our own res_state.

Mixing the ancient non thread safe API (to which _res belongs) with the new (res_ninit() and friends) is pretty stupid. I'll see if I can come up with a proper patch avoiding _res.
Comment 15 Ted Mielczarek [:ted.mielczarek] 2013-10-22 11:13:20 PDT
Comment on attachment 808122 [details] [diff] [review]
exclude broken

I'm okay with this patch, it just seems like a silly approach.
Comment 16 Martin Husemann 2013-11-01 06:08:07 PDT
I had a closer look, and all places where mozilla now calls res_ninit() are not needed at all on NetBSD, and no thread local res_state is needed either.

The NetBSD libc resolver code automatically handles changes to /etc/resolv.conf, so for an application that never uses the low-level resolver API, there is no need to do anything special, getaddrinfo(3), getnameinfo(3) and friends will just work.
Comment 17 Martin Husemann 2013-11-01 06:38:05 PDT
Created attachment 825880 [details] [diff] [review]
Avoid AC_TEST_LINK on systems where we do not want res_ninit
Comment 18 Martin Husemann 2013-11-01 06:54:13 PDT
Landry, can you please check wether OpenBSD handles /etc/resolv.conf changes in libc automatically as well?
And someone (tm) then please fix the comment in Attachment #825880 [details] [diff] before landing accordingly (I can not even parse it, as it is)
Comment 19 Landry Breuil (:gaston) 2013-11-02 00:30:07 PDT
(In reply to Martin Husemann from comment #18)
> Landry, can you please check wether OpenBSD handles /etc/resolv.conf changes
> in libc automatically as well?

I've been told we've been handling that automagically since 10+ years.
Comment 20 Martin Husemann 2013-11-02 04:10:48 PDT
Yeah, the commit in NetBSD was in 2003 as well ;-)
This means the code in effect with HAVE_RES_NINIT undefined is perfect for both OpenBSD and NetBSD. Should I update the patch to adjust the comment, or can you do that before landing manually?

If we'd like to go really fancy we could rename the variable from HAVE_RES_NINIT to something like NEED_MANUAL_RES_NINIT.
Comment 21 Landry Breuil (:gaston) 2013-11-08 23:43:24 PST
(In reply to Martin Husemann from comment #20)
> Yeah, the commit in NetBSD was in 2003 as well ;-)
> This means the code in effect with HAVE_RES_NINIT undefined is perfect for
> both OpenBSD and NetBSD. Should I update the patch to adjust the comment, or
> can you do that before landing manually?

Coming back to this.. you mean the comment in att #825880 ? ted, can you review it ?
I'd put something like 'no need for res_ninit() on NetBSD and OpenBSD'..

I'll also adapt the commit message for att #808122 since it also affects/changes OpenBSD.
Comment 22 Ted Mielczarek [:ted.mielczarek] 2013-11-14 08:57:54 PST
Comment on attachment 825880 [details] [diff] [review]
Avoid AC_TEST_LINK on systems where we do not want res_ninit

Review of attachment 825880 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, way behind on bugmail and reviews. This seems fine.
Comment 23 Landry Breuil (:gaston) 2013-11-16 00:17:04 PST
I had a close look at what happens on OpenBSD, with --cache-file=/dev/null to avoid hitting the systemwide autoconf cache (which has ac_cv_func_res_ninit=no), and the detection of res_init() fails anyway because of missing includes before resolv.h. So no change for us.

/usr/include/resolv.h:137:3: error: array type has incomplete element type
/usr/include/resolv.h:147:18: error: field 'addr' has incomplete type
/usr/include/resolv.h:164:19: error: field 'ina' has incomplete type
/usr/include/resolv.h:165:20: error: field 'in6a' has incomplete type

The patch from att #808122 didnt apply against m-i, so i handfixed it and landed in
https://hg.mozilla.org/integration/mozilla-inbound/rev/362b06bd9363
Comment 24 Jan Beich 2013-11-16 04:11:40 PST
Created attachment 833329 [details] [diff] [review]
consistent includes

On DragonFly and FreeBSD /etc/resolv.conf is read only once per thread like on Linux. Without this patch "nameserver" updates aren't picked up because res_ninit() isn't detected.

So, don't dismiss attachment 810716 [details] [diff] [review] yet. It had r+ and I'm carrying it over.
Comment 25 Jan Beich 2013-11-16 04:14:21 PST
attachment 808118 [details] [diff] [review] of course...
Comment 26 Landry Breuil (:gaston) 2013-11-16 04:31:08 PST
you lost me here. why was it obsoleted then ?
Comment 27 Jan Beich 2013-11-16 06:16:41 PST
Comment on attachment 833329 [details] [diff] [review]
consistent includes

Perhaps, the reviewer got confused which patch(es) is/are the latest after I jumped on Martin's bug with similar issue. Let's slap r+ again to confirm.
Comment 28 :Ms2ger (⌚ UTC+1/+2) 2013-11-17 05:46:37 PST
https://hg.mozilla.org/mozilla-central/rev/362b06bd9363
Comment 29 Ted Mielczarek [:ted.mielczarek] 2013-11-19 06:24:05 PST
Comment on attachment 833329 [details] [diff] [review]
consistent includes

Review of attachment 833329 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, sorry, I was confused as to what patches were current/needed. If I r+ed something you didn't need to re-request review.
Comment 31 Ryan VanderMeulen [:RyanVM] 2013-11-19 10:30:07 PST
https://hg.mozilla.org/mozilla-central/rev/9c5256d1d8a7

Note You need to log in before you can comment on or make changes to this bug.