810716 - HAVE_RES_NINIT is eroneously set on NetBSD

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Martin Husemann]

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]

Attachment: avoid usage of the _res global resolver state in threaded programs on NetBSD

Comment 2

[Jan Beich]

Attachment: 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]

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]

Attachment: Bug 810716 - Make res_ninit() check work on BSDs.

forgot explicit 'return 0' (pre-C99)

Comment 5

[(not currently active) Ted Mielczarek]

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]

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]

Attachment: 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]

Attachment: exclude broken

Comment 9

[Jan Beich]

Attachment: 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]

_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]

(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

[(not currently active) Ted Mielczarek]

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]

Yes, if we do not really test, the hard-coded list could just be in configure.in.

Comment 14

[Martin Husemann]

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

[(not currently active) Ted Mielczarek]

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]

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]

Attachment: Avoid AC_TEST_LINK on systems where we do not want res_ninit

Comment 18

[Martin Husemann]

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)]

(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]

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)]

(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

[(not currently active) Ted Mielczarek]

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)]

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]

Attachment: 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]

attachment 808118 [details] [diff] [review] of course...

Comment 26

[Landry Breuil (:gaston)]

you lost me here. why was it obsoleted then ?

Comment 27

[Jan Beich]

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 (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/362b06bd9363

Comment 29

[(not currently active) Ted Mielczarek]

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 30

[Landry Breuil (:gaston)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c5256d1d8a7

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9c5256d1d8a7