caldwell / build-emacs

Build scripts for www.emacsformacosx.com
http://www.emacsformacosx.com/about
GNU General Public License v3.0
364 stars 61 forks source link

launch.rb is not robust in the presence of unexpected Emacs-* files #111

Closed johnhawkinson closed 2 years ago

johnhawkinson commented 2 years ago

After moving to Monterey earlier this month and applying the Full Disk Access workaround to #94 at https://emacs.stackexchange.com/a/53037 , I finally got around to trying to give Emacs full disk access without granting it to all ruby scripts globally.

My first cut involved a local copy of ruby in the Emacs app bundle, and changing the launcher.rb #! line to use it. Creating a file called Emacs-ruby-wrapper breaks the launcher script:

jhawk@loud-room MacOS % pwd
/Applications/Emacs.app/Contents/MacOS
jhawk@loud-room MacOS % touch Emacs-ruby-wrapper
jhawk@loud-room MacOS % ./Emacs
/System/Library/Frameworks/Ruby.framework/Versions/2.6/usr/lib/ruby/2.6.0/rubygems/version.rb:212:in `initialize': Malformed version number string wrapper (ArgumentError)
    from /System/Library/Frameworks/Ruby.framework/Versions/2.6/usr/lib/ruby/2.6.0/rubygems/version.rb:203:in `new'
    from /System/Library/Frameworks/Ruby.framework/Versions/2.6/usr/lib/ruby/2.6.0/rubygems/version.rb:203:in `new'
    from ./Emacs:48:in `block in <main>'
    from ./Emacs:48:in `map'
    from ./Emacs:48:in `<main>'

jhawk@loud-room MacOS % 

That appears to be triggered by https://github.com/caldwell/build-emacs/blob/b0abf8db6cd186af993542e501dbd16fd0bcf8ca/launch.rb#L48-L51

This is not reasonable and not robust. Dropping an extra Emacs-whatever file in the app bundle should not cause the launcher script to fail. It's too important to be able to execute Emacs all the time without bizarro wrapper script failures. People depend on editors in special ways so more-than-ordinary robustness is required.

I am not a Ruby programmer so I suspect someone else would craft a better fix than I, but if necessary I can try my hand at it.

p.s.: I'm a little concerned at the lack of apparent attention to #94.

caldwell commented 2 years ago

The fix is to call your portable ruby anything other than Emacs-*. Why not just ruby-wrapper? The script is designed to accept an arbitrary number of specifically named Emacs binaries, not random other files that someone might happen to add to the app directory. This is a design choice, I'm not quite convinced it's a bug.

As for #94, I've not addressed it because I don't quite know how to address it. I personally am currently launching my emacs from the Terminal to bypass all that nonsense. I'm kinda sick of Apple's lame security junk breaking valid programs. You'll notice I still don't even notarize the app—it's on my list but every time I look into it the process is just too annoying to deal with and it works "ok" without it (once you know the right click trick).

The portable ruby is interesting, but I think the real solution is one of two things: (a) binary launcher or (b) single fat binary (with no launcher). Apple has been threatening to remove perl, ruby, and python from the OS for a while now, so eventually something will have to change. I've been leaning towards (a), but the Emacs maintainers have made a bunch of strides so that it can be compiled with the minimum OS different than the current OS and doing live feature checks instead of compile time checks. This makes option (b) potentially workable. But I haven't tested it myself such that I feel confident releasing something that works everywhere the current one does. In particular I'm worried about some unixy system libs (ie, non-cocoa) that may be versioned and different across major OS releases. It's also just a lot more (fiddly) work.

johnhawkinson commented 2 years ago

Thanks, David. Yes, of course choosing a different name for locally-added files in the app bundle works around the problem. My concern is for other people who encounter this and to avoid having them run down the diagnostic rabbit hole. I'm not sure I'd focus on whether it is "a bug."

The concern is this is not robust. The wrapper script fails in unexpected ways when presented with unanticipated input. And that input is on a vector that most people would not anticipate the wrapper would look at (generally we can add files to app bundles without consequence). And the error message is unhelpful, because there is inadequate bounds checking when processing the .match() output.

And editors have a special requirement for robustness. People depend on editors in special ways. Indeed, this is probably especially true for Emacs on MacOS, now that the OS no longer ships with a native emacs.

(Indeed, my first expectation when I got the error was was that somehow changing the #! line at the top of the Emacs wrapper had broken it and that I should start by reverting that change. It took me a while to look closely enough at the error to realize it was a consequence of adding new files inside the app bundle; and then as I looked at the code I thought at first it was because I had renamed the original Emacs wrapper to Emacs.dist, although it turned out the wrapper only globbed for Emacs-* not Emacs* so that wasn't picked up.)

But to respond:

Why not just ruby-wrapper?

For what it's worth, the name should be chosen so its purpose is clear when viewed in the System Preferences > Security & Privacy > Privacy: Full Disk Access UI. And that UI only displays the filename portion of the entry (without path), so choosing an explanatory filename is wise. Of course, it still need not start with Emacs (I ended up with usr-bin-ruby-emacs).

As for #94, I've not addressed it because I don't quite know how to address it.

If we're not sure how to solve it, maybe we should do some brainstorming in that Issue — it wasn't apparent reading it that it was blocked because of uncertainty as to how to move forward.

The portable ruby is interesting, but I think the real solution is one of two things: (a) binary launcher or

I admit I'm a little confused why you're referring to it as "portable." But anyhow, yes, I agree that a non-script launcher seems like the proper solution. I'm definitely not suggesting my answer is a good long-term solution, it was intended as a 5-minute quick fix.

johnhawkinson commented 2 years ago

My first cut involved a local copy of ruby in the Emacs app bundle, and changing the launcher.rb #! line to use it. Creating a file called Emacs-ruby-wrapper breaks the launcher script:

It turns out this strategy is also differently not robust. After updating to 12.2, my copy of ruby would fail to execute and dogging int he console logs produces some noise about invalid binary signatures and trust caches. Apparently you can't simply run a 12.1 copy of /usr/bin/ruby under 12.2, for whatever reason.

caldwell commented 2 years ago

There is a new rust launcher now which has a tighter regex when checking for local emacs exes.