[Bug 856072]

[Michael K. Edwards]

(In reply to Doug Turner (:dougt) from comment #27)
> Comment on attachment 563867 [diff] [details] [review]
> Updated patch, with Honeycomb handling
> 
> Review of attachment 563867 [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> looking better.  Please fixup.  mwu should also look at these changes when
> the patch is cleaned up.
> 
> mwu - are you comfortable with the license here?  is it similar to the
> APKOpen stuff?
> 
> ::: memory/mozutils/Makefile.in
> @@ +83,5 @@
> >  ifeq (Android, $(OS_TARGET))
> >  # Add Android linker
> >  EXTRA_DSO_LDOPTS += $(ZLIB_LIBS)
> >  SHARED_LIBRARY_LIBS += $(call EXPAND_LIBNAME_PATH,android,$(DEPTH)/other-licenses/android)
> > +WRAP_LDFLAGS = -Wl,--wrap=getaddrinfo,--wrap=freeaddrinfo,--wrap=gai_strerror
> 
> Why is this needed, but wasn't needed to wrap malloc?

Now that we call __real_getaddrinfo() from __wrap_getaddrinfo(),
libmozutils linking breaks without it.

> ::: other-licenses/android/002-replace-stdio-with-mmap.patch
> @@ +28,5 @@
> > +@@ -94,7 +105,6 @@
> > + #include <netdb.h>
> > + #include "resolv_private.h"
> > + #include <stddef.h>
> > +-#include <stdio.h>
> 
> why this change?

Because the point of the patch is to remove all use of stdio from
getaddrinfo().  The commented-out debug log lines, if restored, should
be changed from stderr to the Android log mechanism (which is useful on
a non-rooted device, unlike stderr).

> @@ +54,5 @@
> > ++
> > ++#define _PSEUDO_FILE_INITIALIZER { -1, 0, MAP_FAILED, 0 }
> > ++
> > ++static void
> > ++_pseudo_fclose(_pseudo_FILE* fp)
> 
> test for a null fp? and elsewhere

Really just needs a "restrict" keyword, since it's never valid to pass
it a NULL pointer.

> @@ +111,5 @@
> > ++    if (maxcopy > bufsize - 1)
> > ++        maxcopy = bufsize - 1;
> > ++    if (maxcopy <= 0)
> > ++        return NULL;
> > ++    //fprintf(stderr, "_pseudo_fgets(): copying up to %d bytes\n", maxcopy);
> 
> remove

remove what?  just the commented-out log line?

> @@ +259,5 @@
> > +@@ -1373,8 +1508,6 @@
> > + 		const struct sockaddr_in6 *addr6 = (const struct sockaddr_in6 *)addr;
> > + 		if (IN6_IS_ADDR_LOOPBACK(&addr6->sin6_addr)) {
> > + 			return 0;
> > +-		} else if (IN6_IS_ADDR_ULA(&addr6->sin6_addr)) {
> 
> what is this change about?

That macro isn't defined by the older headers in the NDK we're using.
It's a side effect of another patch that was already applied to the
version of getaddrinfo.c I started from; we want the rest of that patch.

> @@ +268,5 @@
> > +@@ -1414,8 +1547,6 @@
> > + 		const struct sockaddr_in6 *addr6 = (const struct sockaddr_in6 *)addr;
> > + 		if (IN6_IS_ADDR_LOOPBACK(&addr6->sin6_addr)) {
> > + 			return 60;
> > +-		} else if (IN6_IS_ADDR_ULA(&addr6->sin6_addr)) {
> 
> and this?

ditto

> @@ +278,5 @@
> > + }
> > + 
> > + static void
> > +-_sethtent(FILE **hostf)
> > ++_sethtent(_pseudo_FILE *hostf)
> 
> can't you just
> 
> #undef FILE
> #define FILE _pseudo_FILE
> 
> for this and similar redefs. at the top of this file?  It will safe a bunch
> of these changes.

No.  Note the different number of *s.  I changed the APIs so that the
_pseudo_FILE struct is caller-allocated, in order to avoid the extra
malloc/free.

> @@ +313,5 @@
> > + 	const char *addr;
> > + 	char hostbuf[8*1024];
> > + 
> > +-//	fprintf(stderr, "_gethtent() name = '%s'\n", name);
> > ++	//fprintf(stderr, "_gethtent() name = '%s'\n", name);
> 
> revert this change.

OK; it was just for consistency throughout the commented-out log lines
(which are bad practice IMHO anyway, but whatever).

> @@ +335,5 @@
> > + 		tname = cp;
> > + 		if ((cp = strpbrk(cp, " \t")) != NULL)
> > + 			*cp++ = '\0';
> > +-//		fprintf(stderr, "\ttname = '%s'", tname);
> > ++		//fprintf(stderr, "\ttname = '%s'\n", tname);
> 
> same.

OK

> @@ +368,5 @@
> > + 	name = va_arg(ap, char *);
> > + 	pai = va_arg(ap, struct addrinfo *);
> > + 
> > +-//	fprintf(stderr, "_files_getaddrinfo() name = '%s'\n", name);
> > ++	//fprintf(stderr, "_files_getaddrinfo() name = '%s'\n", name);
> 
> same.

OK

> ::: xpcom/base/nsSystemInfo.cpp
> @@ +48,5 @@
> >  #ifdef ANDROID
> >  #include "AndroidBridge.h"
> > +
> > +extern "C" {
> > +extern volatile int android_sdk_version;
> 
> volatile keyword not needed.  you should not use extern here either.  (this
> is the implicit definition).  you will need extern for the explicit
> declaration (where it is used outside of this file)

A bare "volatile" is never really the right answer, I agree; but I am
concerned about the access getting optimized away, given that the poke
happens from a different thread than the peek.  But I'll take it off if
you'd rather, along with the extern (which I am aware is the implicit
default; I am just in the habit of making it explicit).

-- 
You received this bug notification because you are a member of Mozilla
Bugs, which is subscribed to Mozilla Firefox.
https://bugs.launchpad.net/bugs/856072

Title:
  Bionic domain name functions are not thread-safe on pre-3.0 Android

To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/856072/+subscriptions