758517 - Support for GNU/kFreeBSD and Hurd

Status: RESOLVED FIXED

(NSS :: Build, defect)

Description

[Mike Hommey [:glandium]]

GNU/kFreeBSD is a system with the FreeBSD kernel and a GNU userland (including glibc).
Hurd is vaporware that has some peculiarities, like not having MAXPATHLEN/PATH_MAX.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Support for GNU/kFreeBSD and Hurd

Some parts by Samuel Thibault

Comment 2

[Kai Engert [:KaiE:]]

Comment on attachment 627113 [details] [diff] [review]
Support for GNU/kFreeBSD and Hurd

Helpwanted for getting this code reviewed, in particular that the changes to FREEBL and SHLIBSIGN are correct.

Comment 3

[Kai Engert [:KaiE:]]

Comment on attachment 627113 [details] [diff] [review]
Support for GNU/kFreeBSD and Hurd

Bob is offering to have a look.

Comment 4

[Wan-Teh Chang]

Comment on attachment 627113 [details] [diff] [review]
Support for GNU/kFreeBSD and Hurd

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

Thank you for the patch.  I didn't review the changes to
shlibsign.c and unix_rand.c.  I suggest some changes for
the rest of the files below.

::: security/coreconf/Linux.mk
@@ +105,1 @@
>  	OS_REL_CFLAGS	+= -DLINUX2_0

Linux 2.0 systems are obsolete.  We should simply remove
this block of code.

I verified that no code is testing the LINUX2_0 macro:
http://mxr.mozilla.org/security/search?string=LINUX2_0

@@ +149,5 @@
> +ifeq ($(KERNEL),linux)
> +	ARCH		= linux
> +else
> +	ARCH		= gnu
> +endif

I believe the ARCH variable is defined but not used.
Compare
  http://mxr.mozilla.org/security/search?string=$(CPU_ARCH)
with
  http://mxr.mozilla.org/security/search?string=$(ARCH)

So I suggest that we simply remove this.

Is a non-Linux kernel in this file guaranteed to be GNU?
I think it can also be kFreeBSD, right?

@@ -159,5 @@
>  ifdef _SBOX_DIR
>  LDFLAGS			+= -Wl,-rpath-link,/usr/lib:/lib
>  endif
>  
> -# INCLUDES += -I/usr/include -Y/usr/include/linux

We should simply remove this comment.

@@ +204,2 @@
>  OS_REL_CFLAGS   += -DLINUX2_1
> +endif

We should simply remove this.  No code is testing the
LINUX2_1 macro:
http://mxr.mozilla.org/security/search?string=LINUX2_1

::: security/coreconf/arch.mk
@@ +154,5 @@
>      OS_RELEASE := $(subst ., ,$(OS_RELEASE))
>      ifneq ($(words $(OS_RELEASE)),1)
>  	OS_RELEASE := $(word 1,$(OS_RELEASE)).$(word 2,$(OS_RELEASE))
>      endif
> +    KERNEL = linux

Nit: I think "Linux" looks nicer than "linux".

@@ +157,5 @@
>      endif
> +    KERNEL = linux
> +endif
> +
> +# This check must be last.  Since all uses of OS_ARCH that follow affect only

Nit: this check is not really "last".  We can say
"This check must be after the check for Linux."

@@ +161,5 @@
> +# This check must be last.  Since all uses of OS_ARCH that follow affect only
> +# userland, we can merge other Glibc systems with Linux here.
> +ifneq (, $(filter GNU GNU_%, $(OS_ARCH)))
> +OS_ARCH = Linux
> +OS_RELEASE = 2.6

We should define KERNEL as "gnu" or "GNU" here.  Seems bad
to leave it undefined.

::: security/coreconf/config.mk
@@ +62,5 @@
>  #       one for each OS release.                                      #
>  #######################################################################
>  
>  TARGET_OSES = FreeBSD BSD_OS NetBSD OpenUNIX OS2 QNX Darwin BeOS OpenBSD \
> +              AIX RISCOS WINNT WIN95 WINCE Linux GNU GNU_%

I believe this change is unnecessary because we already
changed OS_TARGET to Linux in arch.mk.  (OS_TARGET is the
same as OS_ARCH by default.)

::: security/nss/lib/softoken/softoken.h
@@ +313,5 @@
>  /* Solaris 8, s9 use PID checks, s10 uses pthread_atfork */
>  
>  #define CHECK_FORK_MIXED
>  
> +#elif defined(LINUX) || defined (__GLIBC__)

I can we can just test __GLIBC__ here.  Can Linux not use
glibc?

::: security/nss/lib/ssl/sslmutex.c
@@ +88,5 @@
>      PR_Lock(pMutex->u.sslLock);
>      return SECSuccess;
>  }
>  
> +#if defined(LINUX) || defined(AIX) || defined(BEOS) || defined(BSDI) || (defined(NETBSD) && __NetBSD_Version__ < 500000000) || defined(OPENBSD) || defined(__GLIBC__)

Please fold this line after "defined(BSDI) ||".

Make the same change to sslmutex.h.

Comment 5

[Mike Hommey [:glandium]]

(In reply to Wan-Teh Chang from comment #4)
> I can we can just test __GLIBC__ here.  Can Linux not use
> glibc?

It can. There's, for instance, at least klibc and uclibc. (And there *was* a gentoo user using firefox with uclibc on #developers a few days ago)

Comment 6

[Robert Relyea]

Comment on attachment 627113 [details] [diff] [review]
Support for GNU/kFreeBSD and Hurd

r- 

I reviewed shlibsign.c and unix_rand.c

There is nothing wrong with the idea's of the change. I have only one issue and two nits:

shlibsign.c - you do need to free the new buffer you allocate, even though shlibsign exits later. All of the NSS tools are checked for memory leaks so we can detect potentical links in NSS itself. A leak in the tool could turn tinderbox orange.

NITS.
shlibsign.c consistently uses PR_Malloc and friends from NSPR. You should use that instead of malloc/realloc/free.
unix_raand.c - freebl consistenly uses PORT_Alloc/PORT_Free. Please use those functions for your allocation.

Other than that, those two patches would be fine.

bob

Comment 8

[Jon Boden]

Attachment: nss.diff

Hi!

Why are porting changes for two different systems mixed up in the same bug? This just makes it harder to get the patch reviewed & committed: The dynamic reallocation changes everyone is concerned about are only needed on Hurd.

I'm attaching a new patch for GNU/kFreeBSD. It drops the dynamic reallocation part and implements all the changes in the build system requested by Wan-Teh Chang.

Can we move forward with this? It's been almost 4 years :-)

Comment 9

[Kai Engert [:KaiE:]]

Jon, you should set a review request for Bob.

Comment 10

[Jon Boden]

Thanks Kai. I think I've done that, waiting for Bob's review now :-)

Comment 11

[Jon Boden]

Hi

I see that Bob has added review+. Does this mean the patch can be committed now?

If not what else is needed?

Thanks!

Comment 12

[Jon Boden]

Hi!

How is this going? Is there something more I can do about this patch?

Comment 13

[Jon Boden]

If my last patch is good, would be cool if someone can commit it.

Otherwise please tell me what else is needed :-)

Comment 14

[Kai Engert [:KaiE:]]

Sigh. Jon, I apologize that everyone has ignored this for so long. We all get too much bugmail, and just now I have the chance to go through some of the mail that has queued up.

For escalating, when you're waiting for someone, instead of just adding a comment, it can be helpful to set the "need more information" flag for a person (bottom of bugzilla page).

The patch no longer applies cleanly. I will try to merge it, test it and if it works, commit it. If this doesn't happen within the next few days, please ask me for needinfo.

Comment 15

[Kai Engert [:KaiE:]]

try build:
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=68ba2943c37eb4d4eb0c592b6ac1761ba06767ea

Comment 16

[Kai Engert [:KaiE:]]

https://hg.mozilla.org/projects/nss/rev/d2828c27b052