1538102 - [PATCH] OpenBSD null environ handling in native messaging

Status: RESOLVED INCOMPLETE

(Toolkit :: Async Tooling, defect, P2)

Description

[Anton Vilhelm Ásgeirsson]

Attachment: openbsd-environ-null.patch

After applying #1519750 to omni.ja and testing native messaging I noticed that the environ wasn't getting populated. What happens is that the stringArray() function accepts options.environ as null. wraps it in a char pointer and returns it. The char pointer is not NULL so the call to execve will not inherit the parent's environ.

I see a couple of ways to address this:

• stringArray()should return null when it's param is null. I have included a patch for this.
• The code in subprocess_worker_unix.js could check if options.environment is null before passing it as a param to stringArray(). This way maybe makes more sense if there are some edge cases in other OSes around a null environ that I'm not aware of.

Testing was done on OpenBSD 6.5 snapshots on firefox 65 and 66.

Comment 1

[Anton Vilhelm Ásgeirsson]

Comment on attachment 9052806 [details] [diff] [review]
openbsd-environ-null.patch

I couldn't set the review flag to '?' as suggested on IRC, so I added '+' and '?' in the feedback flag.
I hope that's okay.

Comment 2

[Kris Maglione [:kmag]]

Comment on attachment 9052806 [details] [diff] [review]
openbsd-environ-null.patch

>diff -r b2f1edb41241 toolkit/modules/subprocess/subprocess_worker_common.js
>--- a/toolkit/modules/subprocess/subprocess_worker_common.js    Thu Mar 21 12:41:13 2019 +0200
>+++ b/toolkit/modules/subprocess/subprocess_worker_common.js    Thu Mar 21 22:04:48 2019 +0000
>@@ -79,7 +79,7 @@
> 
>   /**
>    * Creates a null-terminated array of pointers to null-terminated C-strings,
>-   * and returns it.
>+   * and returns it. If the strings param is null, this function returns null.
>    *
>    * @param {string[]} strings
>    *        The strings to convert into a C string array.
>@@ -87,6 +87,10 @@
>    * @returns {ctypes.char.ptr.array}
>    */
>   stringArray(strings) {
>+    if (!Boolean(strings)) {
>+      return null;
>+    }
>+
>     let result = ctypes.char.ptr.array(strings.length + 1)();
> 
>     let cstrings = strings.map(str => ctypes.char.array()(str));

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 9052806 [details] [diff] [review]
openbsd-environ-null.patch

Review of attachment 9052806 [details] [diff] [review]:
-----------------------------------------------------------------

I'm a bit leery of this. I agree that having `stringArray` mangle null into a string pointer is bad. But that's also clearly not what's happening here, since `strings.length` would throw if `strings` was null. In fact, I can't think of a case where your patch would have the effect you describe, since any value that evaluates to `false` should throw either when we try to access its `length` property, or when we try to call its `map` function.

Either way, though, I think the correct fix is to make `getEnvironment` correctly populate the environment object on OpenBSD. That's necessary for the pathSearch code, anyway, so lots of stuff won't work if it doesn't.

::: toolkit/modules/subprocess/subprocess_worker_common.js
@@ +87,4 @@
>     * @returns {ctypes.char.ptr.array}
>     */
>    stringArray(strings) {
> +    if (!Boolean(strings)) {

No need for the explicit `Boolean` cast. Just `!strings` has the same effect.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:Yoric, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Do I understand correctly that this bug breaks features on *BSD?

Comment 6

[David Teller [:Yoric] - still alive but not very active]

No answer from reporter, so I guess it's not high priority.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Redirect a needinfo that is pending on an inactive user to the triage owner.
:Gijs, since the bug has high priority, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 8

[:Gijs (he/him)]

Without more info it's not clear how to move forward here. We can reopen if more info becomes available.