315793 - shlibsign incorrectly runs from mozilla/security/nss/cmd/shlibsign

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Julien Pierre]

shlibsign is executed from a source-level directory. If it crashes, the core file is dumped in that source directory. When multiple builds are being done from different platforms, if more than one crashes, the core file gets overwritten. 

The solution is to run shlibsign from a different directory, such as mozilla/security/nss/cmd/shlisbign/$OBJDIR or mozilla/dist/$OBJDIR/bin .

Comment 1

[Julien Pierre]

Attachment: run signing script from $(OBJDIR)

Comment 2

[Wan-Teh Chang]

Comment on attachment 202448 [details] [diff] [review]
run signing script from $(OBJDIR)

This patch assumes that $(OBJDIR), $(DIST), and $(NSPR_LIB_DIR)
are all relative pathnames.  This is not true.  If BUILD_TREE
is set (and is an absolute pathname), $(OBJDIR) and $(DIST) will
be absolute pathnames.  NSPR_LIB_DIR can also be set to an
absolute pathname.  This problem can be fixed by converting
these three pathnames to absolute pathnames.  We have an
abspath function defined in coreconf.  Or you can do
$(shell cd $(OBJDIR); pwd), and so on.

The change from $(OS_TARGET) to ../$(OS_TARGET) is incorrect
because $(OS_TARGET) is not a pathname.

Comment 3

[Julien Pierre]

Attachment: working fix

Comment 4

[Wan-Teh Chang]

Comment on attachment 202879 [details] [diff] [review]
working fix

The ../sign.sh relative pathname is not correct if
BUILD_TREE is set.

I encourage you to come up with a patch that will fix
bug 275112 and this bug.  Bug 275112 already has two
patches (fix #2 and alternate fix #2) that will change
the current working directory to $(DIST)/lib before
invoking shlibsign on Windows.  Like $(OBJDIR), $(DIST)/lib
is a platform-dependent directory, so changing directory
to $(DIST)/lib will also fix this bug.  It seems that
you just need to generalize one of those two patches
to non-Windows platforms.

Comment 5

[Julien Pierre]

Attachment: add SRCDIR to reference sign.sh / sign.cmd

Wan-Teh,

I looked at bug 275112. It is a closely related problem - due to the execution directory of shlibsign.exe. However, the solution is much more restrictive on Windows, XP since not just any platform-dependent directory will do. And unfortunately, I didn't see agreement between timeless and you on the various patches.

This bug affects all platforms, and we don't even do any official builds on Windows XP (we still build windows on NT 4.0). Bug 275112 is specific to Windows XP and is thus lower priority (and correctly marked P3).

My inclination is to fix this bug here with the patch attached to the Makefile.
I think bug 275112 can be fixed with the alternate fix #2 or some variant of it that only touches sign.sh and not Makefile. Thus, there is no conflict, and bug 275112 can be fixed separately from this bug.

Comment 6

[Julien Pierre]

Setting version to 3.8, since that was the first NSS version to have shlibsign. Marking P2 for 3.11 .

Comment 7

[Wan-Teh Chang]

Comment on attachment 202940 [details] [diff] [review]
add SRCDIR to reference sign.sh / sign.cmd

I found that our abspath funciton is not good enough.
If we are not on AIX, OS/2, or Windows, NEED_ABSOLUTE_PATH
is not defined, and abspath will simply return its input
argument:

 ifdef NEED_ABSOLUTE_PATH
 abspath = $(if $(findstring :,$(1)),$(1),$(if $(filter /%,$(1)),$(1),$(PWD)/$(1)))
 else
 abspath = $(1)
 endif

Sorry to have led you down this path, Julien.

My recommendation for this bug is WONTFIX.  This is a
lot of work and can potentially make the makefile rule
very unreadable, just so we don't overwrite the shlibsign
core file dumped by another platform.

>+SRCDIR = $(call abspath,.)

It's better to say

  SRCDIR := $(shell pwd)

Comment 8

[Julien Pierre]

Attachment: fix abspath function to work on all platforms

Wan-Teh,

I believe this change to rules.mk should suffice to make the abspath function work on all platforms. Combined with the previous patch, this should fix the bug . This bug has repeatedly caused a lot of confusion and wasted many people's time in investigation of the wrong core files, so I would like to get it fixed.

Comment 9

[Julien Pierre]

Comment on attachment 203046 [details] [diff] [review]
fix abspath function to work on all platforms

Note that this patch will require gmake 3.79 or later on all platforms in order to support functions. Currently, some platforms are still building with 3.76 from 1997. However, seeing as gmake 3.79 itself is already 4.5 years old, I think it's OK for this requirement to be imposed.

Comment 10

[Julien Pierre]

Comment on attachment 202940 [details] [diff] [review]
add SRCDIR to reference sign.sh / sign.cmd

Wan-Teh, please review this patch again together with  this one : https://bugzilla.mozilla.org/attachment.cgi?id=203046 .

Comment 11

[Wan-Teh Chang]

Comment on attachment 202940 [details] [diff] [review]
add SRCDIR to reference sign.sh / sign.cmd

On second thought, I suggest that we do the
conversion to absolute pathnames in the sign.sh
shell script.  The abspath makefile function is
only defined for AIX, OS/2, and Windows for a
reason: only those platforms' debuggers can
take advantage of full pathnames of the source
files.  If we define abspath on all platforms,
we will passing full pathnames of the source
files to the compilers on all platforms.  It
seems wrong to make the fix for this bug require
that change.

Comment 12

[Julien Pierre]

Wan-Teh,

These patches should not affect the path of the C source files passed to the compiler on Unix platforms. NEED_ABSOLUTE_PATH is still only set for the platforms that use it (AIX, Windows, OS/2), and the rule that compiles C sources still calls abspath only when needed because of the ifdef NEED_ABSOLUTE_PATH. But the rule that calls shlibsign now always calls abspath on all platforms.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 203046 [details] [diff] [review]
fix abspath function to work on all platforms

r=nelson
I haven't reviewed the definition of abspath fully, but since it hasn't changed, I believe it is no less correct than before.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 202940 [details] [diff] [review]
add SRCDIR to reference sign.sh / sign.cmd

r=nelson.  A couple thoughts:
1. would be nice we we could wrap these two REALLY LONG lines
2. With and without this patch, the shell must be named "sh" for this to work.  Seems to me that there's an envivonment variabled named ${SHELL} that should be used instead of "sh".  But using "sh" is no less correct than before.

Comment 15

[Julien Pierre]

Nelson, thanks for the review.

I checked both patches on to the tip for 3.12 . I wrapped the long lines in the shlibsign Makefile.

Checking in security/coreconf/rules.mk;
/cvsroot/mozilla/security/coreconf/rules.mk,v  <--  rules.mk
new revision: 1.67; previous revision: 1.66
done
Checking in security/nss/cmd/shlibsign/Makefile;
/cvsroot/mozilla/security/nss/cmd/shlibsign/Makefile,v  <--  Makefile
new revision: 1.16; previous revision: 1.15
done

Marking fixed.

Comment 16

[Wan-Teh Chang]

Comment on attachment 203046 [details] [diff] [review]
fix abspath function to work on all platforms

> else
>-abspath = $(1)
>+# everything else
>+PWD = $(shell pwd)
>+endif
> endif

You should use := to define PWD.

Comment 17

[Julien Pierre]

Thanks for catching that. I checked this in to the tip .

Checking in rules.mk;
/cvsroot/mozilla/security/coreconf/rules.mk,v  <--  rules.mk
new revision: 1.68; previous revision: 1.67
done

Comment 18

[Julien Pierre]

Also checking in to NSS_3_11_BRANCH under new single review rule.

Checking in coreconf/rules.mk;
/cvsroot/mozilla/security/coreconf/rules.mk,v  <--  rules.mk
new revision: 1.66.2.1; previous revision: 1.66
done
Checking in nss/cmd/shlibsign/Makefile;
/cvsroot/mozilla/security/nss/cmd/shlibsign/Makefile,v  <--  Makefile
new revision: 1.13.2.1; previous revision: 1.13
done

Comment 19

[Julien Pierre]

Also updating target to 3.11.1 .

Comment 20

[glen beasley]

*** Bug 334560 has been marked as a duplicate of this bug. ***

Comment 21

[Julien Pierre]

The checkin on the branch didn't have the long lines wrapped in the Makefile. I have now remedied this for consistency.

Checking in Makefile;
/cvsroot/mozilla/security/nss/cmd/shlibsign/Makefile,v  <--  Makefile
new revision: 1.13.2.2; previous revision: 1.13.2.1
done