Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

emscripten with AudioWorklets: performance.now and crypto #13224

Open
jllodra opened this issue Jan 10, 2021 · 7 comments
Open

emscripten with AudioWorklets: performance.now and crypto #13224

jllodra opened this issue Jan 10, 2021 · 7 comments

Comments

@jllodra
Copy link

jllodra commented Jan 10, 2021

Hi, after building an audio library (libopenmpt) to WASM, using the es6 module approach,
I find that it generates js code that relies on performance.now and crypto, so it does not work on an Audio Worklet context.

The workaround I used to make it work was using Date.now, and using a polyfill for crypto (for random number generation).

What would be the best way of sorting that out? Could emscripten generate the code for that to work?

Thanks

@manxorist
Copy link
Contributor

I'm the maintainer of libopenmpt (https://github.com/OpenMPT/openmpt/).

We worked around the performance.now problem for now (by avoiding std::chrono::high_resolution_clock on the C++ side).

The issue with crypto remains. The only thing libopenmpt really wants/needs here is std::random_device to seed a PRNG. std::random_device in C++ is not specified to provide cryptographically safe random numbers (/dev/random (and somewhat less /dev/urandom) in Linux are, though). As I understand it, the C++ implementation in libc++ uses the emulated /dev/urandom, which fails hard in emscripten if cryptographically safe random numbers are not available.

libopenmpt itself would even gracefully handle a C++ exception in std::random_device (which would be allowed behaviour from the C++ standards perspective), however this point is not even reached because emscripten aborts (rightfully so, from the perspective of dev/urandom) here: https://github.com/emscripten-core/emscripten/blob/master/src/library.js#L2858 .

I'm not sure what a good solution would be here, in particular because in libopenmpt, I cannot detect at compile-time if we are targeting to run inside an AudioWorkletProcesssor (where crypto and thus std::random_device is unavailable) or not (where everything works normally).

@manxorist
Copy link
Contributor

As I understand it, the C++ implementation in libc++ uses the emulated /dev/urandom, which fails hard in emscripten if cryptographically safe random numbers are not available.

Reading further, it could actually also use the getentropy() library function:
libcxx/random.cpp

unsigned
random_device::operator()()
{
    unsigned r;
    size_t n = sizeof(r);
    int err = getentropy(&r, n);
    if (err)
        __throw_system_error(errno, "random_device getentropy failed");
    return r;

or

unsigned
random_device::operator()()
{
    unsigned r;
    size_t n = sizeof(r);
    char* p = reinterpret_cast<char*>(&r);
    while (n > 0)
    {
        ssize_t s = read(__f_, p, n);
        if (s == 0)
            __throw_system_error(ENODATA, "random_device got EOF");
        if (s == -1)
        {
            if (errno != EINTR)
                __throw_system_error(errno, "random_device got an unexpected error");
            continue;
        }
        n -= static_cast<size_t>(s);
        p += static_cast<size_t>(s);
    }
    return r;
}

I'm not sure which implementation is used in emscripten, however both of them will work fine if getentropy() would return an error or if read() from /dev/urandom would return 0 (no data) instead of calling abort().

@sbc100
Copy link
Collaborator

sbc100 commented Jan 11, 2021

Seems reasonable to return error rather than abort in getentropy(). Would you like to upload a patch to that effect?

What do you mean "both of them will work fine" if we return an error? It looks like they would both __throw_system_error.. do you have higher level code that catches that exception? (And are you building with C++ exceptions enabled (which is costly)).

@manxorist
Copy link
Contributor

Seems reasonable to return error rather than abort in getentropy(). Would you like to upload a patch to that effect?

I would rather prefer if someone with actual experience in Javascript would write such a patch.

What do you mean "both of them will work fine" if we return an error? It looks like they would both __throw_system_error.. do you have higher level code that catches that exception?

Yes. C++ std::random_device::operator() is specified to throw when random number generation is impossible for any reason (see https://en.cppreference.com/w/cpp/numeric/random/random_device/operator()). libopenmpt handles this exception and falls back to a PRNG seeded with the current time (which is not perfect, but mostly good enough for our use case) (https://github.com/OpenMPT/openmpt/blob/master/common/mptRandom.cpp#L213). Emscripten would not be the only platform where std::random_device throws. This can also happen on platforms without getentropy() inside of containers when /dev/random is not provided.

And are you building with C++ exceptions enabled (which is costly).

Yes, libopenmpt relies on C++ exceptions to work correctly (we basically require a complete C++ implementation), and emscripten builds of libopenmpt thus have exceptions enabled (-s DISABLE_EXCEPTION_CATCHING=0).

@jllodra jllodra closed this as completed Mar 9, 2021
@manxorist
Copy link
Contributor

So why was this closed? There is NOTHING solved here yet. Emscripten still does not implement a fully working C++ standard library usable in AudioWorklets. The fact that libopenmpt works around emscripten bugs now is not a reason to close the emscripten issue.

@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

@stale stale bot added the wontfix label Apr 16, 2022
@manxorist
Copy link
Contributor

Well, it looks like

$getRandomDevice: function() {
still aborts, so this issue still needs fixing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants