517417 - access violation: while compiling xulrunner tries to test for Mercurial repositories above its build dir

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Arne Babenhauserheide]

User-Agent:       Mozilla/5.0 (compatible; Konqueror/4.3; Linux) KHTML/4.3.1 (like Gecko)
Build Identifier: xulrunner-1.9.1.3

I tried to build xulrunner on my Gentoo which creates a sandbox for it in /var/tmp/portage/net-libs/xulrunner-1.9.1.3/work/ 

Write access outside this sandbox is not allowed (else it gets an access violation). 

Now I have a Mercurial repository in /var (to version some important files like my mpd playlists) and xulrunners check for a Mercurial repository seems to propagate outside the sandbox and try to write in my /var/.hg dir: 

...
F: open_wr
S: deny   
P: /var/.hg/wlock
A: /var/.hg/wlock
R: /var/.hg/wlock
C: /usr/bin/python2.6 /usr/bin/hg identify 
...
F: open_wr
S: deny   
P: /var/nxTE_D
A: /var/nxTE_D
R: /var/nxTE_D
C: /usr/bin/python2.6 /usr/bin/hg identify 



Reproducible: Always



Expected Results:  
The check for the hg repo shouldn't propagate outside the build dir to avoid that sandbox violation. 

I verified that the hg repo was the reason by moving it away before building. When there wasn't a hg repo in /var/, building worked without problems. 

Additional information is available in the according Gentoo bug: http://bugs.gentoo.org/show_bug.cgi?id=284673

Comment 1

[(not currently active) Ted Mielczarek]

We run "hg -R $topsrcdir identify" to find the changeset you're building from. I guess hg will look for a repo in a higher directory, and fail if that's readonly. I wouldn't take a patch for this. Put your source directory somewhere else.

Comment 2

[Arne Babenhauserheide]

Does that mean I'll have to patch it myself to only do that check if $topsrcdir/.hg exists? 

It would simply mean to use 

if [ -d "$topsrcdir/.hg" ]; then hg -R $topsrcdir id; fi

instead of the simple hg id, since you know after all, where the .hg *must* be.

Comment 3

[Arne Babenhauserheide]

PS: I use a Gentoo system. Changing the source dir is nontrivial and systemwide.

Comment 4

[(not currently active) Ted Mielczarek]

Yes, I'm saying that your problem is such an edge case that I would not take a patch to fix it.

Comment 5

[Arne Babenhauserheide]

I know that you won't (easily?) take the patch, but since a clean fix (without if...fi) changes only 6 characters: 

You run "cd $(topsrcdir) ; hg identify" while the way which would work with higher up hg repos is to call either "cd $(topsrcdir) ; hg identify ." or "hg identify $(topsrcdir)". 

You do that in crashreporter/tools/symbolstore.py

There you call "hg id -i srcdir" which makes sure that other hg dirs can't interfere. Also it is a lot faster when there is no hg repo than letting hg traverse the filesystem tree (on my system the inexact way takes about 3 seconds when there is no hg repo above the srcdir. The exact way takes less than 300ms). 

You also pass the path in browser/locales/Makefile.in

So this fix unifies the way you identify the revision. 

It is either for you, or I'll ask the Gentoo devs if they'll take it. If you decide that my reasoning is good (and the 6 character change is warranted for a edge case), you can simply import the patch via "hg import". It's done in the 1.9.1 branch. 

# HG changeset patch                                                  
# User Arne Babenhauserheide <[e-mail]>                         
# Date 1253306010 -7200                                               
# Node ID be4db1d5647cbee6ea6bda8ff2e5992db16f1773                    
# Parent  71163cab3efb1d098ad0e0ac832e40b7a7d54ac4                    
Bug 517417.  Don't let hg identify traverse into higher directories.  

diff --git a/browser/app/Makefile.in b/browser/app/Makefile.in
--- a/browser/app/Makefile.in                                 
+++ b/browser/app/Makefile.in                                 
@@ -72,7 +72,7 @@ GRE_BUILDID = $(shell $(PYTHON) $(topsrc    
                                                              
 DEFINES += -DGRE_MILESTONE=$(GRE_MILESTONE) -DGRE_BUILDID=$(GRE_BUILDID)

-SOURCE_STAMP := $(shell cd $(topsrcdir) ; hg identify 2>/dev/null | cut -f1 -d' ')
+SOURCE_STAMP := $(shell cd $(topsrcdir) ; hg identify . 2>/dev/null | cut -f1 -d' ')
 ifdef SOURCE_STAMP
 DEFINES += -DMOZ_SOURCE_STAMP="$(SOURCE_STAMP)"
 endif
diff --git a/toolkit/content/Makefile.in b/toolkit/content/Makefile.in
--- a/toolkit/content/Makefile.in
+++ b/toolkit/content/Makefile.in
@@ -58,7 +58,7 @@ DEFINES += \
   -DCPPFLAGS="$(CPPFLAGS)" \
   $(NULL)

-CHANGESET := $(shell cd $(topsrcdir) && hg identify 2>/dev/null | cut -f1 -d' ')
+CHANGESET := $(shell cd $(topsrcdir) && hg identify . 2>/dev/null | cut -f1 -d' ')
 ifdef CHANGESET
 DEFINES += -DSOURCE_CHANGESET="$(CHANGESET)"
 endif
diff --git a/toolkit/xre/Makefile.in b/toolkit/xre/Makefile.in
--- a/toolkit/xre/Makefile.in
+++ b/toolkit/xre/Makefile.in
@@ -250,7 +250,7 @@ DEFINES     += -DHAVE_USR_LIB64_DIR
 endif
 endif

-SOURCE_STAMP := $(shell cd $(topsrcdir) && hg identify 2>/dev/null | cut -f1 -d' ')
+SOURCE_STAMP := $(shell cd $(topsrcdir) && hg identify . 2>/dev/null | cut -f1 -d' ')
 # strip a trailing slash from the repo URL because it's not always present,
 # and we want to construct a working URL in buildconfig.html
 # make+shell+sed = awful

Comment 6

[:Gavin Sharp [email: [e-mail]]]

That patch looks reasonable. Have you tested to ensure that it works (i.e. about:buildconfig shows the right revision)?

You should attach it as an attachment and request review (set the review flag to "?" and enter ted's email address).

Comment 7

[Jory A. Pratt [:anarchy]]

Attachment: ensure we only look inside our src_dir and not rest of filesystem

Exact same patch as in bug report, just attached and added to bug for review and inclusion. Patch has been tested and causes no danger.

Comment 8

[Arne Babenhauserheide]

@Gavin: Sorry for not answering - my mailbox got flooded and I seem to have overlooked your mail...

Comment 9

[Serge Gautherie (:sgautherie)]

Could you attach a hg patch?

Comment 10

[Jory A. Pratt [:anarchy]]

(In reply to comment #9)
> Could you attach a hg patch?

I will create an hg patch based on trunk as soon as possible.

Comment 11

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/1a1fbe3a6393

Comment 12

[Arne Babenhauserheide]

many thanks!

Comment 13

[Serge Gautherie (:sgautherie)]

While there, I wonder why they use 2 '&&' and 1 ';':
shouldn't it be he same everywhere?

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 405285 [details] [diff] [review]
ensure we only look inside our src_dir and not rest of filesystem

"approval1.9.2=?":
"approval1.9.1.4=?":
No risk, faster and safer.

Comment 15

[Doug Turner (:dougt)]

This change broke my Fennec / Maemo build.  Not sure why tinderbox's are still building.


/usr/bin/python2.5 /home/dougt/builds/mobile/mozilla-central/toolkit/xre/make-platformini.py --buildid=20091025215535 --sourcestamp=hg  print        aliases: /home/dougt/builds/mobile/mozilla-central/config/milestone.txt > platform.ini
Traceback (most recent call last):
  File "/home/dougt/builds/mobile/mozilla-central/toolkit/xre/make-platformini.py", line 24, in <module>
    (milestoneFile,) = args
ValueError: too many values to unpack

Comment 16

[(not currently active) Ted Mielczarek]

What version of hg is that? Does it print some error or help text if you run "hg identify ." in the sourcedir?

Comment 17

[Samuel Sidler (old account; do not CC)]

Comment on attachment 405285 [details] [diff] [review]
ensure we only look inside our src_dir and not rest of filesystem

Pushing out approval request.

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 405285 [details] [diff] [review]
ensure we only look inside our src_dir and not rest of filesystem

Doesn't seem like the kind of fix we have to take on the stable branch. 1.9.1 approval denied.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 405285 [details] [diff] [review]
ensure we only look inside our src_dir and not rest of filesystem

I think we'd rather take bug 515792 than just this bug, but probably not worth bothering for 1.9.2 at this point.

Comment 20

[Serge Gautherie (:sgautherie)]

(In reply to comment #13)
> While there, I wonder why they use 2 '&&' and 1 ';':
> shouldn't it be he same everywhere?

Bug 515792 will get rid of that too ;->