11.17 KB, patch
Mark Mentovai: first-review+
Chase Phillips: second-review+
Chris Beard: approval1.8b4+
|Details | Diff | Splinter Review|
204 bytes, text/plain
339 bytes, text/plain
2.05 KB, patch
Chase Phillips: first-review+
|Details | Diff | Splinter Review|
Currently the mac xulrunner tbox is red. There are two reasons for this: 1) the tinderbox config is looking for dist/XULRunner.app/Contents/MacOS/xulrunner-bin when it should be looking for dist/XUL.framework/Versions/Current/xulrunner-bin 2) We need to ship the XUL framework as an installer.pkg, not an app bundle (still inside a DMG). step 1 is for chase/coop, step 2 is for me. Step 2) unfortunately involves some hackery with commands executed as root: in one part of the install process, it is necessary to chmod -R root:admin <a directory> which requires root privs (and it would be good to "undo" this task after packaging is finished). To do this automatically from a makefile probably involves putting a suid script somewhere.
Created attachment 192388 [details] [diff] [review] Make a XULRunner installer .pkg and then DMG it, rev. 1 [checked in]
Created attachment 192390 [details] The source I'm using for setuid chown_root This or something like it will need to be installed as a setuid program (owned by root) on the tinderbox build machine.
Created attachment 192391 [details] The source I'm using for setuid chown_revert Same same, for chown_revert
pax files, as in *.pkg/Contents/Archive.pax.gz are just cpio archives. How do you feel about fixing up the uid and gid of each entry in the cpio header after running PackageMaker? It can be done by pipeline in a trivial script. I think I prefer that to suid wrappers, and it can be integrated into the build. We'd need to re-run mkbom to get a proper Archive.bom afterwards. No biggie.
Mark, that sounds fine, if you can help write it, at least providing significant examples... I don't know how. Does mkbom take the CPIO file as input?
I thought it did, but it seems to only operate on expanded directories. I'll see if it's trivial to transform the bom file too - if not, I guess there's no choice other than making Chase install setuid helpers. The scripts would be a piece of cake, if it's possible, I'll smack 'em together later.
Unfortunately, the bom file format isn't publicly documented, nor are the APIs into the bom framework. I can see the proper fields in the bom file, but I don't think it's wise to go poking around like this, and I don't think reverse-engineering it is an effective use of time. Apple really isn't leaving us with much of a choice here. Thanks, Apple.
(In reply to comment #0) > Currently the mac xulrunner tbox is red. There are two reasons for this: > > 1) the tinderbox config is looking for > dist/XULRunner.app/Contents/MacOS/xulrunner-bin when it should be looking for > dist/XUL.framework/Versions/Current/xulrunner-bin > > step 1 is for chase/coop, step 2 is for me. Made some intial changes to the build scripts, and we've gone green: http://tinderbox.mozilla.org/showlog.cgi?log=XULRunner/1123854540.12639.gz&fulltext=1 Still some packaging problems at the end of the process that I'm looking into: building file list ... link_stat "/builds/tinderbox/XR-Trunk/Darwin_7.9.0_Depend/mozilla/dist/Mozilla.app" failed: No such file or directory
(In reply to comment #8) > Still some packaging problems at the end of the process that I'm looking into: > > building file list ... link_stat > "/builds/tinderbox/XR-Trunk/Darwin_7.9.0_Depend/mozilla/dist/Mozilla.app" > failed: No such file or directory This will apparently be fixed by step 2.
Created attachment 192528 [details] [diff] [review] tinderbox build script changes for XULRunner/MacOS [checked in] This patch does the following: * changes $DistBin for XULRunner on MacOS; * puts /sw/bin at the front of $PATH on MacOS; * fixes perl function warning in encode_log
Mark, unless you're holding out hope for some sort of BOM manipulation can you review the patch I've got?
Comment on attachment 192388 [details] [diff] [review] Make a XULRunner installer .pkg and then DMG it, rev. 1 [checked in] So, .bom is a bomb. This is good for now. Eventually, more of the .pkg making logic should probably be integrated into packager.mk, the .pkg will be a better place for the EULA than the .dmg, etc. Notes on this patch: You've got to chmod package-stage so that it's world-readable and executable where appropriate. Permissive umasks aren't guaranteed, and the user that builds the .pkg needs to be able to get at the files and directories owned by root:admin. Doing this before $(CHOWN_ROOT) is good: chmod -R a+rX,u+w,go-w $(DIST)/package-stage && \ You probably also want to clear the s/t bits. It would look really bad to accidentally ship something that installs setuid programs when not necessary (although a few major software publishers do it and apparently nobody has noticed or cares.) This error mesage and the one for CHOWN_REVERT: +CHOWN_ROOT ?= $(error CHOWN_ROOT must be set to a setuid script.) are misleading because of the use of the word "script." I'm not sure if you meant to seek review of two utilities, my r+ only applies to this patch file with the above changes. I do have concerns about the implementation of chown_root and chown_revert. I'd be frightened as hell to have either installed setuid on my system, even if restricted to a sandboxed build user. chown_root, because it doesn't clear the s-bits. The safest way to go would be to have chown_root do its own recursion, chowning with fchown, and clearing any s-bits with fchmod. chown_revert should at least check ownership of the parent directory. Building and installing them obviously can't be part of the build, so I don't know if you plan on putting either utility's source in cvs. I think it's a good idea for them to be in the tree, but there's no way they could be added in their current states. As for not integrating them into the tree and putting them on the tinderbuilders manually, I guess that's Chase's call.
P.S. that's a capital X in my chmod.
Somebody is welcome to make chown_root/chown_revert a lot safer; unix system security is not my specialty. It should probably only work on specified hardcoded paths (make sure that the argument starts with /builds or /tinderbox or something). Adding a chmod -s (or C equivalent) sounds like a good idea as well.
Comment on attachment 192388 [details] [diff] [review] Make a XULRunner installer .pkg and then DMG it, rev. 1 [checked in] This patch is checked in.
Coop, this is all yours now ;-) The tinderbox variables need to be updated so that instead of building xpinstall/packager they build xulrunner/installer (just like browser/installer or mail/installer)... and you need to set CHOWN_ROOT and REVERT_ROOT in the environment and compile/install them as setuid root programs.
columbia's config has been updated and these apps installed. It should cycle shortly.
(In reply to comment #16) > Coop, this is all yours now ;-) > > The tinderbox variables need to be updated so that instead of building > xpinstall/packager they build xulrunner/installer (just like browser/installer > or mail/installer)... and you need to set CHOWN_ROOT and REVERT_ROOT in the > environment and compile/install them as setuid root programs. s/REVERT_ROOT/CHOWN_REVERT/
The build appears to have failed with the following error: ... exit $SAVED 2005-08-22 12:51:35.820 PackageMaker ** building a package at /builds/tinderbox/XR-Trunk/Darwin_7.9.0_Depend/mozilla/dist/xulrunner-1.9a1.en-US.mac.pkg ** dyld: /Developer/Applications/Utilities/PackageMaker.app/Contents/MacOS/PackageMaker malformed library: /Developer/SDKs/MacOSX10.2.8.sdk/System/Library/Frameworks/CoreServices.framework/Frameworks/CarbonCore.framework/CarbonCore (not a Mach-O library file, bad filetype value) make: *** [libs] Error 133 make: *** [libs] Error 2 make: *** [all] Error 2 No files to copy ... The full build log can be seen at: http://tinderbox.mozilla.org/showlog.cgi?log=XULRunner/1124738640.16774.gz&fulltext=1 Let me know if you'd like more info from the system.
Some versions of Apple's tools are a little overzealous about using $NEXT_ROOT. The libraries in the SDK are only stubs and are intended for linking only, not loading. This needs: unset NEXT_ROOT; \ before $(CHOWN_ROOT)
Mark, the change you suggest is for mozilla/, not the build scripts, right? Handing back to email@example.com who is handling the mozilla/ side of things on this one. Please reassign to me if the requested change needs to happen on the build system instead.
Chase, the unset NEXT_ROOT fix is for the tree. :bs landed it shortly after I suggested it. I assume this is still open for 1.8 branch lovin' - but if there's no branch xulrunner tinderbuilder, I guess there's no build script work to be done. Your reassignment seemed not to stick, I'm trying again.
Fixed on 1.8branch as well (with the NEXT_ROOT patch).
I have checked in the source of chown_root.c and chown_revert.c into mozilla/build/macosx/permissions