Replace the "eval" statement introduced as part of bug 1518172 with a better solution.

I'm afraid we cannot avoid the use of "eval", but it might be safe in our scenario.

Because we do background generation of OTR keys, we must pass a void* pointer from the main thread to the worker thread.

The OTR library interface clearly defines which API calls must be called on the main thread, and which ones may be called on a background thread.

When creating a new key, a "preparation function" must be called from the main thread, which returns a void* to raw data managed by the external OTR C library. This pointer may be given to a background, or JS worker thread.

The pointer is represented as a JS CData object, declared as
let newkey = new ctypes.void_t.ptr();

Trying to pass this JS object to a worker results in an exception:
DataCloneError: The object could not be cloned.

Based on the references I found, cloning of this type is unsupported.

The code works around that, by calling newkey.toSource(), which provides a source code representation of the raw pointer value. This string can be passed to the background worker. In the worker's code, "eval" is used to convert that source code representation back into an object, which can then be passed to the external OTR library.

eval(parameter) is considered risky, if the parameter may have have been produced as a result of user input, or from the network. However, in our scenario, that's impossible. Yes, the raw function pointer was defined by an external library. But we know what type it has.

I assume the sequence of
let newkey = new ctypes.void_t.ptr();
-> giving newkey.address() to someone else, who can set the value of void*
let str = newkey.toSource()
will always result in a string that represents a void*, without the risk that it could be anything else.

The string won't ever contain dynamic instructions, it will represent a static value.

Given that we strictly control what parameter is given to the eval function, it seems acceptable to use it.

It's necessary to use it, assuming we want to keep the background key generation functionality.

Of course, if someone knows of a different solution for passing a raw ctypes void* to a JS worker thread, please educate me.

Florian, Patrick, what do you think? Given the explanation, is the use of eval acceptable, and could we close this bug as wontfix?

Just for reference, the code discussed is here: https://searchfox.org/comm-central/rev/ee05c0d9006626f95dfbb1b6a79bed84e2b90e98/chat/content/otrWorker.js#13

What does the string returned by newkey.toSource() look like?

(In reply to Florian Quèze [:florian] from comment #4)

What does the string returned by newkey.toSource() look like?

ctypes.voidptr_t(ctypes.UInt64("0x123456789abc"))

(In reply to Kai Engert (:kaie:) from comment #5)

(In reply to Florian Quèze [:florian] from comment #4)

What does the string returned by newkey.toSource() look like?

ctypes.voidptr_t(ctypes.UInt64("0x123456789abc"))

Ok, so how about transfering only 0x123456789abc then?

(In reply to Florian Quèze [:florian] from comment #6)

Ok, so how about transfering only 0x123456789abc then?

What happens when we'll build on a different CPU architecture? We might use the wrong pointer size.

Detecting the size of the pointer seems straightforward. If you transfer it as a string, you can check the length of the string.

Florian, I would highly recommend to keep the string as prepared by toSource.

If we try to extract the pointer, try to derive the exact type to use, and try to reassemble that into the correct type on the other size, we'll be reinventing the wheel of creating a serialization/deserialization/ of pointer values.

If you're worried that this contains anything else but a pointer, we could doublecheck on the receiving size, that it's indeed a pointer, by checking that it starts with ctypes.voidptr_t(ctypes., and that it ends with ")), and that the thing in the middle has a max size of a 128 bit pointer length.

(Which will break once we get to a 256 bit OS ;-) )
I'd recommend to write platform compatible code.

Also note that fiddling with raw pointer values has the risk to go wrong. If we make any mistake converting the numbers back into a pointer, we'll cause the C OTR library to access random memory, and we'll either crash, or do worse.

"0x123456789abc" can be transfered as a string, then on the receiving side you have ctypes.voidptr_t(ctypes.UInt64(data)) instead of eval(data)

You don't know if it's 32 or 64 or 128 or 256

That's easy to guess, but if you don't want to guess you can transfer that information too.

You don't know if future architectures will still use UInt128 or something else

Sorry, I had missed an important part of your comments. My arguments were based on the incorrect assumption that using "eval" would still be necessary, and that we'd just have a smaller dynamic portion of the string, when constructing it.

Thanks for your clarification on IRC, that by using the syntax you suggested in comment 12, we could potentially avoid the use of "eval".

This still requires that we make assumptions how the output of toSource() is structured. I don't know if it will always be same, on Windows, Mac, and on the various Linux hardware architectures, present and future.

toolkit/components/ctypes/tests/unit/test_jsctypes.js
contains the following test:
Assert.equal(v.toSource(), 'ctypes.voidptr_t(ctypes.UInt64("0x0"))');

So maybe it's safe to assume the same string representation is used across all platforms that Mozilla currently supports.

lint, etc

Depends on D32838

patch ready to land

This patch is on top of the patch from bug 1552177.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0cddb77c7079
dont' use "eval" in the OTR code. r=mkmelin

TB 68 beta:
https://hg.mozilla.org/releases/comm-beta/rev/0ef29ea2f3c06debefcf9d0dd6847f02257dc67a