Created attachment 73627 [details] [review]
api changes for js188

Created attachment 73628 [details] [review]
build patch for js188

Given the dlopen stuff, I would not have though linking of libmozjs would be required, however I was getting 'undefined reference' for macros defined in jsapi.h (such as JSVAL_NULL) while building.

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

This effectively reverts the dynamic loading work from http://cgit.freedesktop.org/polkit/commit/?id=34cb6353b9affd6c04ea480df5fc39d0ca72319d

The commit message doesn't really explain why, but from what I understand, it was primarily requested by people making "minimal cloud images" and the like, where they may not actually be making use of polkit at runtime.  

(Sadly these people would be better served by compiling from source, but let's leave that aside).

So I think before we move forward with this patch, we'll have to figure out how to keep the dynamic loading.


There is a secondary high level concern, which was discussed in https://bugzilla.gnome.org/show_bug.cgi?id=690982 - basically just bumping the mozjs requirement is going to be painful.

Given the current scope of changes, it does look to me like polkit could get by with #ifdef.  I can investigate this and see.

(In reply to comment #4)
> This effectively reverts the dynamic loading work from
> http://cgit.freedesktop.org/polkit/commit/
> ?id=34cb6353b9affd6c04ea480df5fc39d0ca72319d
> 
> The commit message doesn't really explain why, but from what I understand,
> it was primarily requested by people making "minimal cloud images" and the
> like, where they may not actually be making use of polkit at runtime.  
> 
> (Sadly these people would be better served by compiling from source, but
> let's leave that aside).
> 
> So I think before we move forward with this patch, we'll have to figure out
> how to keep the dynamic loading.
> 


I could never get dynamic loading to work with the new spidermonkey library, probably because the mozilla build scripts seem to strip out the --export-dynamic flag. 

build/autoconf/pkg.m4:            ## Remove evil flags like -Wl,--export-dynamic

Ok, so I looked into this.  It has nothing to do with -export-dynamic, the problem is now JSVAL_NULL is a global data symbol, and not a #define anymore.  See:

extern JS_PUBLIC_DATA(const jsval) JSVAL_NULL;

So we'd need to dlsym() it like everything else.  Or theoretically we could just hack up our own local copy of:

const jsval JSVAL_NULL  = IMPL_TO_JSVAL(BUILD_JSVAL(JSVAL_TAG_NULL,      0));


Regardless though, what I'm thinking now it's that it'd be a **whole** lot simpler if we just had a build-time option --enable-javascript-rules.  Then GNOME (and its downstreams) could turn that on, and "tiny" downstreams (minimal cloud images) could disable it.

For binary package-based systems like Fedora and Debian that want to be a universe of bits that can be dynamically assembled per use case, they could build polkit twice, and have e.g. "polkit" and "polkit-nojs" packages.

David, Miroslav, what do you think?

Er, Miloslav, sorry, lots of tyops today.

Actually, I think we should just drop the whole "load JS interpreter dynamically" approach and just link statically. It never really made much sense and was added more or less as a compromise when I was at Red Hat in order to appease some people concerned about having js.rpm installed. Something about that this package is often updated for security issues and this tends to scare some people. I can't remember the details.

Anyway, I don't think it's unreasonable to ask Red Hat to just carry patches for this themselves if they want to hang on to that feature (probably want for RHEL7). Thoughts?

(In reply to comment #9)
> Actually, I think we should just drop the whole "load JS interpreter
> dynamically" approach and just link statically.
> It never really made much
> sense and was added more or less as a compromise when I was at Red Hat in
> order to appease some people concerned about having js.rpm installed.
> Something about that this package is often updated for security issues and
> this tends to scare some people. I can't remember the details.
> 
> Anyway, I don't think it's unreasonable to ask Red Hat to just carry patches
> for this themselves if they want to hang on to that feature (probably want
> for RHEL7). Thoughts?

When the system spidermonkey (libjs) package gets updated for security, like for an CVE, then polkit would need a recompile too.
And when user runs something like gjs linked against dynamic libjs, he'd get another copy running within polkit?
I don't see any benefits in linking it statically, only massive headacke.
Would be something we'd need to patch out at distribution level since it'd be against the QA policies of every distribution I know of -- "Useless static linking"

(In reply to comment #10)

Sorry, I meant "just link dynamically" (as opposed to dlopen() as we do today) instead of "just link statically. (FWIW, I agree it would be non-sense to link _statically_.)

(In reply to comment #11)
> (In reply to comment #10)
> 
> Sorry, I meant "just link dynamically" (as opposed to dlopen() as we do
> today) instead of "just link statically. (FWIW, I agree it would be
> non-sense to link _statically_.)

phew!  i'm really glad to hear this.  :-)

for source based distributions it doesn't make much difference at all if it's NEEDED or dlopen()'d but I can understand how it can be useful for binary distributions if the purpose is to keep libjs optional
i'll stop talking now.

(In reply to comment #9)
> Actually, I think we should just drop the whole "load JS interpreter
> dynamically" approach and just link statically. 

That's my strong preference as well, because it means consistent polkit semantics, and more compiler checking.

> It never really made much
> sense and was added more or less as a compromise when I was at Red Hat in
> order to appease some people concerned about having js.rpm installed.

AFAIK the major concerns were different and the dlopen() stuff didn't resolve them, so...

> Anyway, I don't think it's unreasonable to ask Red Hat to just carry patches
> for this themselves if they want to hang on to that feature (probably want
> for RHEL7).

... for Fedora I intend to have a hard dependency on the JS intepreter in any case, even if the code continues to use dlopen().


(I also want to resurrect pkla, which is another story, but having a hard dependency on the JS interpreter will actually make it easier to carry as a local patch.  So one more eason to support just linking to the interpreter.)

(In reply to comment #13)

> AFAIK the major concerns were different and the dlopen() stuff didn't
> resolve them, so...

Hm, really?  I thought it came out of the minimal cloud push.  Well OK, I'll take a look at linking dynamically then, and once we do that, if someone cares a --disable-javascript build time option would be pretty easy.

(In reply to comment #13)
> (I also want to resurrect pkla, which is another story, but having a hard
> dependency on the JS interpreter will actually make it easier to carry as a
> local patch.  So one more eason to support just linking to the interpreter.)

Please don't. Please also don't ship it as a patch in Fedora as it will make it look like upstream supports it (which we of course won't). If all you want is a file-format instead of JS (and I'd argue such a desire is misguided given the nature of JS), I would suggest coming up with a simpler format than .pkla (and parse it from a JS rules files, at least for a first cut).

(In reply to comment #15)
> (In reply to comment #13)
> > (I also want to resurrect pkla, which is another story, but having a hard
> > dependency on the JS interpreter will actually make it easier to carry as a
> > local patch.  So one more eason to support just linking to the interpreter.)
> 
> Please don't. Please also don't ship it as a patch in Fedora as it will make
> it look like upstream supports it (which we of course won't).

With a mandatory JS interpreter it can be done as a .rule that calls out to external helpers, which is as separate as it can get while still having a single consistent behavior within the distribution.

> If all you
> want is a file-format instead of JS (and I'd argue such a desire is
> misguided given the nature of JS), I would suggest coming up with a simpler
> format than .pkla (and parse it from a JS rules files, at least for a first
> cut).

Well, long-term I want an unified system-wide ACL/permissions mechanism, and neither .pkla nor anything else ran from the JS backend can be it (or at least it's not known that it can be it at this time).  So any file format would ideally be medium-term thing (3-5 years) and at that point it's just easier for everyone if we don't introduce a new format, especially a new Fedora-only format.

(In reply to comment #14)
> (In reply to comment #13)
> 
> > AFAIK the major concerns were different and the dlopen() stuff didn't
> > resolve them, so...
> 
> Hm, really?  I thought it came out of the minimal cloud push.  Well OK, I'll
> take a look at linking dynamically then,

This turns out to be just matter of reverting 34cb6353b9affd6c04ea480df5fc39d0ca72319d and fixing up a few conflicts.  Patches to come.

Created attachment 77911 [details] [review]
0001-jsauthority-Use-JSVAL_NULL-rather-than-0-struct-init.patch

Created attachment 77912 [details] [review]
0002-Revert-Dynamically-load-libmozjs185.so-and-cope-with.patch

Created attachment 77913 [details] [review]
0003-jsauthority-Work-with-mozjs-17.0-too.patch

All compiler warnings fixed, lightly tested in gnome-ostree buildmaster.

I guess this doesnt matter too much, however JS_VERSION has been deprecated and will be removed in the next major mozjs24 release. It has been replaced by MOZJS_MAJOR_VERSION and friends.

(In reply to comment #21)
> I guess this doesnt matter too much, however JS_VERSION has been deprecated
> and will be removed in the next major mozjs24 release. It has been replaced
> by MOZJS_MAJOR_VERSION and friends.

Yeah, that seems like an easy thing to deal with when updating to the next version.

(In reply to comment #20)
> Created attachment 77913 [details] [review] [review]
> 0003-jsauthority-Work-with-mozjs-17.0-too.patch
> 
> All compiler warnings fixed, lightly tested in gnome-ostree buildmaster.

All three look good, test suite passes with both versions.

(I wouldn't mind a version that didn't have #ifdefs and just required the newer library,  OTOH I assume we don't expect many changes to the backend anyway, so the #ifdefs don't hurt much, and might make it easier on users, so both alternatives are fine with me.)

(In reply to comment #23)
> (In reply to comment #20)
> > Created attachment 77913 [details] [review] [review] [review]
> > 0003-jsauthority-Work-with-mozjs-17.0-too.patch
> > 
> > All compiler warnings fixed, lightly tested in gnome-ostree buildmaster.
> 
> All three look good, test suite passes with both versions.

Thanks, pushed all 3 patches.  Let's open a new bug for any non-js17 discussion that was in this bug.

> (I wouldn't mind a version that didn't have #ifdefs and just required the
> newer library,  OTOH I assume we don't expect many changes to the backend
> anyway, so the #ifdefs don't hurt much, and might make it easier on users,
> so both alternatives are fine with me.)

Yeah, it wasn't very hard so I decided to make things a bit easier for downstreams.  In contrast for gjs it would have been *really* ugly, so we just branched and declared the mozjs185 legacy.