Misleading C++ exception handling example

[schaumb]

Version of emscripten/emsdk:
Documentation - all version from here.

std::string getExceptionMessage(intptr_t exceptionPtr) {
  return std::string(reinterpret_cast<std::exception *>(exceptionPtr)->what());
}

According to the documentation, this code snippet appears to be a handler function for getExceptionMessage. However, it is much simpler than the current implementation found here.

The documentation also mentions that "this example code will only work for thrown statically allocated exceptions" but in reality, it only works if the exception type is exactly std::exception. It will not work (according to the standard C++) if the exception is a child of std::exception because is_pointer_interconvertible_base_of_v<std::exception, std::[any other std exception]> evaluates to false. Therefore, the reinterpret casting is not safe and results in undefined behavior.

Another issue is that this code can be exploited using the following structure:

struct BaseException {
  virtual ~BaseException() = default;
  virtual const char* hack() const noexcept { return "HACKED"; }
};
struct MyEx :  BaseException, std::exception {
  virtual const char* what() const noexcept { return "This won't be returned"; }
};

throw MyEx();

---

We are using Emscripten with the following switches: -s DISABLE_EXCEPTION_CATCHING=1 -fno-rtti. This means that we don't have a bound __get_exception_message, but we still have exceptions and we want to retrieve the reason using .what(). We thought that this code snippet would be sufficient for handling .what(), but it turns out it's not. Therefore, this part of the documentation is misleading.

[sbc100]

IIUC when you build with -sDISABLE_EXCEPTION_CATCHING=1 that means that throws turn into traps, and you can't actually catch anything.. Am I missing something? How are you getting handle to the exception if catching is disabled?

[schaumb]

@sbc100
The title of the linked documentation is "Handling C++ exceptions from JavaScript."

The -sDISABLE_EXCEPTION_CATCHING=1 flag disables catching only, which means that throws do not turn into traps.

The following flags are NOT set: -sDISABLE_EXCEPTION_THROWING, -fno-exceptions, and -fwasm-exceptions. Therefore, the default behavior for exceptions is as follows:

function ___cxa_throw(ptr, type, destructor) {
  var info = new ExceptionInfo(ptr);
  info.init(type, destructor);
  exceptionLast = ptr;
  uncaughtExceptionCount++;
  throw exceptionLast;
}

In the code where we want to guess the exception message, it is something like this (where f is a C function):

_call(f) {
  return (...params) => {
    try {
      return f(...params);
    } catch (e) {
      if (Number.isInteger(e)) {
        let address = parseInt(e, 10);
        let cMessage = this.module._vizzu_errorMessage(address); // getExceptionMessage
        let message = this.module.UTF8ToString(cMessage);
        throw new Error("error: " + message);
      } else {
        throw e;
      }
    }
  };
}

---

You need to set the -s ASSERTIONS=0 flags too in order to generate this ___cxa_throw

[sbc100]

I see, so your code is working correctly, this is simply a request to clarify the documentation?

Perhaps @aheejin has some thoughts on this?

[sbc100]

It seems like that documentation should be updated since now we have __get_exception_message and getExceptionMessage helper functions the can get referred to. That documentation should probably have been updated when those helper functions we added back in #16343.

Can you explain a little more what you mean by "we don't have a bound __get_exception_message:" What happens if you use the function? I guess because RTTI is missing you don't get things like ->name.. but does the rest of the function work, or just is crash, or something else?

[schaumb]

When we set the -sDISABLE_EXCEPTION_CATCHING=1 flag in Emscripten, the getExceptionMessage function disappears.

When we attempted to enable the -sEXPORT_EXCEPTION_HANDLING_HELPERS=1 flag, the if-else statement at lines 2905 to 2909 in the emcc.py prevents the function from being exported, leading to errors.

It is likely that the condition in this section is not properly configured. Instead of checking the disabled catching, it should examine the disabled throwing flag.

[sbc100]

I think you likely correct.

Perhaps it can be confusing because that getExceptionMessage is specifically designed for handling/catching exceptions but DISABLE_EXCEPTION_CATCHING implies that exceptions cannot be caught. I suppose DISABLE_EXCEPTION_CATCHING only applies to catching in native code.. and in this case you are doing your catching in JS.. which one might is something that is not supported when DISABLE_EXCEPTION_CATCHING.

In any case.. this seems worth fixing.

[aheejin]

I can make EXPORT_EXCEPTION_HANDLING_HELPERS available when DISABLE_EXCEPTION_THROWING is set and DISABLE_EXCEPTION_CATCHING is not set, but this will add more functions to .js files and pull dependent library functions in https://github.com/emscripten-core/emscripten/blob/main/system/lib/libcxxabi/src/cxa_exception_emscripten.cpp. Wouldn't it increase the code size for all programs, given that DISABLE_EXCEPTION_THROWING=0 && DISABLE_EXCEPTION_CATCHING=1 is the default state if you don't specify any EH options? @sbc100, are these additionally included functions in DEFAULT_LIBRARY_FUNCS_TO_INCLUDE gonna be DCE'd if you don't end up using them, or they end up increasing code size?

[sbc100]

Won't the code size increases will only effect those who opt into EXPORT_EXCEPTION_HANDLING_HELPERS?

Function that get added to DEFAULT_LIBRARY_FUNCS_TO_INCLUDE, but are not used anywhere should get DCE'd yes. As long as they are not also exported (e.g. via EXPORTED_FUNCTIONS).

[aheejin]

But if I build a program using -sEXPORT_EXCEPTION_HANDLING_HELPERS, getExceptionMessage is in JS code, even if it is not used:

emcc -fwasm-exceptions -sEXPORT_EXCEPTION_HANDLING_HELPERS test.cpp -o test.js

Then test.js contains getExceptionMessage and its dependencies. Even with -O2 or -O3. Doesn't this mean this function and all its dependencies (some JS functions and some Wasm functions in libc++abi) will be added to wasm/js file unconditionally?

[sbc100]

But if I build a program using -sEXPORT_EXCEPTION_HANDLING_HELPERS, getExceptionMessage is in JS code, even if it is not used:

emcc -fwasm-exceptions -sEXPORT_EXCEPTION_HANDLING_HELPERS test.cpp -o test.js

Then test.js contains getExceptionMessage and its dependencies. Even with -O2 or -O3. Doesn't this mean this function and all its dependencies (some JS functions and some Wasm functions in libc++abi) will be added to wasm/js file unconditionally?

Oh, that might be because EXPORT_EXCEPTION_HANDLING_HELPERS also adds getExceptionMessage to EXPORTED_FUNCTIONS. So it can't ever be GC'd.

In any case, anyone who opts into -sEXPORT_EXCEPTION_HANDLING_HELPERS is signing up to pay that code size cost.. that is kind of exactly what the option says.. "hey I need these helpers, please export them and don't GC them". I think that is WAI.

[schaumb]

But if I build a program using -sEXPORT_EXCEPTION_HANDLING_HELPERS, getExceptionMessage is in JS code, even if it is not used:

emcc -fwasm-exceptions -sEXPORT_EXCEPTION_HANDLING_HELPERS test.cpp -o test.js

Then test.js contains getExceptionMessage and its dependencies. Even with -O2 or -O3. Doesn't this mean this function and all its dependencies (some JS functions and some Wasm functions in libc++abi) will be added to wasm/js file unconditionally?

@aheejin This solution only works when we switch on the -fwasm-exceptions option. However, this is not viable for us since we need to support older browsers. Therefore, we are unable to use the getExceptionMessage feature.

---

And another problem is that this switch adds 50k size to the binary which is unacceptable to our users.

[sbc100]

But if I build a program using -sEXPORT_EXCEPTION_HANDLING_HELPERS, getExceptionMessage is in JS code, even if it is not used:

emcc -fwasm-exceptions -sEXPORT_EXCEPTION_HANDLING_HELPERS test.cpp -o test.js

Then test.js contains getExceptionMessage and its dependencies. Even with -O2 or -O3. Doesn't this mean this function and all its dependencies (some JS functions and some Wasm functions in libc++abi) will be added to wasm/js file unconditionally?

@aheejin This solution only works when we switch on the -fwasm-exceptions option. However, this is not viable for us since we need to support older browsers. Therefore, we are unable to use the getExceptionMessage feature.

I think there is some confusion here.. and perhaps it is mine, but IIUC -sEXPORT_EXCEPTION_HANDLING_HELPERS does work in both exception handling modes, not just one.

Perhaps you thought that because @aheejin used -fwasm-exceptions in her example command line that is was required? .. but that is not the case.

[schaumb]

It is not working as I wrote in my upper comment:

When we attempted to enable the -sEXPORT_EXCEPTION_HANDLING_HELPERS=1 flag, the if-else statement at lines 2905 to 2909 in the emcc.py prevents the function from being exported, leading to errors.

It is likely that the condition in this section is not properly configured. Instead of checking the disabled catching, it should examine the disabled throwing flag.

[schaumb]

Can you reopen it, or should I create a new ticket?

[aheejin]

We can consider allowing EXPORT_EXCEPTION_HANDLING_HELPERS when DISABLE_EXCEPTION_CATCHING == 1 && DISABLE_EXCEPTION_THROWING == 0. But you also said this is not an option because it increases the code size. And I checked, and it indeed increases code size more than 50k even at -O3, which currently is hard to fix because enabling getExceptionMessage pulls in a lot of dependencies. So what's your suggestion to solve this?

[schaumb]

I replaced emcc.py:2908 library_exceptions.js:313 and library_exceptions.js:400 to check for EXCEPTION_THROWING. Additionally, I only exported the ___get_exception_message function.

The ___get_exception_message function not only generates the output of what(), but it also demangles the exception type name. The 50k code comes from the demangler and not from the what() function resolving.

So we need only the what() function resolving which can be found here: cxa_exception_js_utils.cpp:__get_exception_message:74-83.

[aheejin]

Yeah right. That was meant to give some more info on the type of the exception, especially in case the exception is not a subclass of std::exception so it doesn't have what. We can possibly create two different functions for doing this, but not sure how much demand there's gonna be. If you can submit a PR for that, that would be great.

[sbc100]

It sounds like that are two different issues. Firstly that this doesn't currently work, and secondly that it adds a lot of code size. I suggest we fix the first issue first, and with a test.

Then we can separately consider how to add a non-demanding version. I'm somewhat averse to adding this extra complexity.. I would love it is there was way to do it without adding a separate. Perhaps we can find a way to have it controlled by the existing DEMANGLE_SUPPORT setting?

[aheejin]

It sounds like that are two different issues. Firstly that this doesn't currently work, and secondly that it adds a lot of code size. I suggest we fix the first issue first, and with a test.

No it currently works as intended. The intention was EXPORT_EXCEPTION_HANDLING_HELPERS was only meant to be used either with DISABLE_EXCEPTION_CATCHING=0 or -fwasm-exceptions. We can probably relax this restriction, but given that the OP is only requester of this and even he is not satisfied with the implications on the code size, I'm not sure whether to pursue it. I think what we can do is to split the type-returning function and what()-returning function, but this is mostly for a very specific need who wants 1. only what() output 2. doesn't use exception handling 3. can't accept code size increase.

Then we can separately consider how to add a non-demanding version. I'm somewhat averse to adding this extra complexity.. I would love it is there was way to do it without adding a separate. Perhaps we can find a way to have it controlled by the existing DEMANGLE_SUPPORT setting?

It doesn't seem to work, because the demangling function is used by __get_exception_message.

[sbc100]

Then we can separately consider how to add a non-demanding version. I'm somewhat averse to adding this extra complexity.. I would love it is there was way to do it without adding a separate. Perhaps we can find a way to have it controlled by the existing DEMANGLE_SUPPORT setting?

It doesn't seem to work, because the demangling function is used by __get_exception_message.

What I meant was that we could perhaps have -sDEMANGLE_SUPPORT=0 somehow control the behaviour of getExceptionMessage so that folks who use that setting didn't pay the code size cost.

[aheejin]

Then we can separately consider how to add a non-demanding version. I'm somewhat averse to adding this extra complexity.. I would love it is there was way to do it without adding a separate. Perhaps we can find a way to have it controlled by the existing DEMANGLE_SUPPORT setting?

It doesn't seem to work, because the demangling function is used by __get_exception_message.

What I meant was that we could perhaps have -sDEMANGLE_SUPPORT=0 somehow control the behaviour of getExceptionMessage so that folks who use that setting didn't pay the code size cost.

We can probably do that, but that's gonna increase one more dimension for the variations for the library builds. We already have except/noexcept/ , debug/ , mt/ , and all combinations of these. Would it be desirable to add demangle/ on top of these?

[sbc100]

Could we make two versions of __get_exception_message and have the getExceptionMessage call one or the other based on DEMANGLE_SUPPORT?

[aheejin]

Like, you call different versions in JS based on the option? I think we could... but not sure how large the demand would be.