Add basic Intel SGX support
Duktape is great for embedded environments, and so I wanted to try and run it in Intel SGX. There is some special configuration needed and refactoring string formatting in a few places, but otherwise works just fine.
Summary of changes:
- Add a new platform
platform_intelsgx. The order is important to be immediately above Linux and Windows. Intel SGX enclaves are compiled under Linux or Windows with a special C library (tlibc; trusted libc) but otherwise are indistinguishable from other object files. Therefore the Linux and Windows detection will trigger. - Automatic detection that we're compiling an enclave is really tricky.
There is no official documentation that describes if there is a macro you can check to see if compiling against tlibc; I did manage to find
_TLIBC_CDECL_and a few others fromsys/cdefs.hwhich might be used for this purpose but I decided it's best to prefer manual versus automatic detection. In order to hint that this is the Intel SGX build either defineINTELSGXmanually or use--platform=intelsgxin theconfigure.pyinvocations. - Intel SGX enclaves do not have access to syscalls, and therefore they can't get information about the current time and locale timezone offset. This makes it impossible to run Duktape without any intervention, but luckily this boils down to defining two macros
DUK_USE_DATE_GET_NOWandDUK_USE_DATE_GET_LOCAL_TZOFFSET. These are mandatory and well documented, and compilation fails with a preprocessor error if they are not added. - Intel SGX's tlibc does not include the "unsafe" string parsing and formatting functions. It only uses the "safe" ones. Meaning that all uses of
DUK_SPRINTFneeded to be refactored intoDUK_SNPRINTF, but that was an easy affair. In general it's my opinion that going forward the unsafe functions be completely "banned" from the Duktape code, and be replaced with safe ones. - Add
DUK_USE_STANDARDIZED_POINTER_ENCODINGoption (default: false) that encodes all pointers in a "standardized" way i.e. in lowercase hexadecimal encoding as the value is laid out in memory. In order to implement this, the functionsduk_encode_pointer_cstrandduk_decode_pointer_cstrwere added toduk_util_misc.c. They have also been replaced wherever the%pformat specifier was used directly or indirectly (except for debug statements, of course). By enabling this option, Duktape no longer usesDUK_SSCANF. - Added a few things to
.gitignore, most importantlyruntests/package-lock.jsonwhich is added by npm when tests are run locally. It may be a good idea to remove this from.gitignoreif we want to pin down versions. - Changed invocation of
pythonwithinruntests/runtests.jstopython2.7since running theecmatestwith Python 3 failed with error. These days Python 3 is behindpython, while 2.7 can be invoked withpython2.7.
This PR is not complete I'm still testing and changing things, but discussions can begin IMO.
Remaining things to complete:
-
apitestfails whenDUK_USE_STANDARDIZED_POINTER_ENCODINGbecause of the hardcoded expectation for0xdeadbeef.ecmatestsucceeds, tho. Possible solutions:- Test expectations get passed through
jadeor some other preprocessor so they can be configured before being run. - All
0xdeadbeefoccurrences in expectations withintests/apiare replaced with a configurable value. - Change the standardized format, though I really like the simplicity of 1:1 memory layout in hex.
- Test expectations get passed through
- Add new doc page for how to compile for Intel SGX.
- Write instructions for testing.
Tests are failing because conditional undef DUK_USE_JX is not possible. The configuration tools read the undef but not the elif defined(DUK_F_INTELSGX) surrounding it.
Here are some possible solutions to the problem:
- Change reading and formatting pointers to functions for that purpose:
Then these can be set to different values for Intel SGX or other constrained environments.#define DUK_PTR2STR(buf,sz,ptr) DUK_SNPRINTF(buf,sz,"%p",ptr) #define DUK_STR2PTR(buf,sz,ptr) DUK_SSCANF(buf,"%p",ptr) - Knowingly leave the
DUK_SSCANFuse inDUK_USE_JXsections, but add documentation and example configuration that disablesDUK_USE_JXfor Intel SGX. - Define platform independent encoding of pointers without the use of
printfandscanffamilies.
Thanks, interesting stuff! I'll look into the changes as soon as I get a chance.
I agree banning plain sprintf() would be nice. The reason sprintf() is currently used is that snprintf() has portability issues, e.g. it's not available on legacy platforms and sometimes interpretations before C99 differ, especially handling of truncation. So there are pros and cons in each case.
I'd be OK with requiring snprintf(), it's probably the better compromise. Users running pre-C99 would then need to implement or borrow one but that's not a big deal. This could be made easier by making it easy to use the minimal replacements https://github.com/svaarala/duktape/tree/master/extras/minimal-printf via config.
I decided to add a new option DUK_USE_STANDARDIZED_POINTER_ENCODING which when enabled will encode pointers in a "standard" way, i.e. read the pointer value as it is laid out in memory and write it to a string in lowercase hex.
This PR is getting a little large, maybe once we agree on things I can break it up into smaller ones.
Re: forcing DUK_F_INTELSGX from outside, there are existing platforms where automatic detection is not possible and an external define is used to force detection, see for example https://github.com/svaarala/duktape/blob/master/config/helper-snippets/DUK_F_AMIGAOS.h.in (for VBCC).
It would be best for the define not to have a DUK_F_ prefix, because the configuration model uses those defines purely for internal purposes. Using just -DINTELSGX would be consistent with handling of e.g. Amiga + VBCC.
But if it would be clearer, maybe forced platforms could be handled using a convention like DUK_PLATFORM_AMIGA, DUK_PLATFORM_INTELSGX, etc.
I'll try to review the diff tomorrow night, assuming it is ready for comments.
Yes, ready for comments @svaarala.
I agree on the point for DUK_F_INTELSGX and will produce changes toward that approach this weekend.
@hf I finally got some time to do a review on the changes. Mostly just trivia but there's one probable snprintf() truncation issue and AUTHORS needs a fix. With these fixed and the commits squashed this is ready for merging. Thanks for the detailed work! :+1:
@svaarala Thanks for reviewing. I tried to get all indent issues, but not sure how I missed these. Maybe think about setting up clang-format to get rid of issues like these?
I'll investigate more about the snprintf, and adjust code so that it works good. I like the MEMBASED name more, so I'll go with that one.
What's the ideal style for void**? :sweat_smile:
Is it void* *?
Still many broken indents. Something is wrong with my vim, it's confused by this codebase.
What's the ideal style for
void**?
In the current codebase it would simply be void **.
@svaarala Thanks for reviewing. I tried to get all indent issues, but not sure how I missed these. Maybe think about setting up clang-format to get rid of issues like these?
I've gone through uncrustify, astyle, and clang-format, and none of them have enough controls to get the results I would want (which is not surprising). But it's probably best to compromise on the result so that code style would be automatic, so I'll merge in something for 3.0.x release.
This was one major issue: https://stackoverflow.com/questions/38620019/can-clang-format-align-a-block-of-defines-for-me. It's fixed in clang-format-9 but that may not be available on the host so the autoindenting step will need to be dockerized.
Also a good idea is to add .editorconfig since this codebase uses tabs (and I prefer spaces, but that is irrelevant). My vim struggled a bit, so I did a refactor of my dotfiles to fix it. :)
Adding a .editorconfig sounds good to me. I'll steer clear of debating tabs vs spaces ;-)
Hey @svaarala I finally managed to fix all outstanding issues. Would you mind checking the logic I used in the unresolved comments before merging?
TODOs post PR:
- https://github.com/svaarala/duktape/pull/2219#discussion_r373690188
I'll take a look - after that commits should be squashed for merging (a single commit is fine).
@hf Went through the changes and there's just some minor trivia left, see review comments.
Once you address them, you can just squash the commits into one, and I'll then merge the changes. I'll fix up whatever trivia is left while working on the snprintf() handling generally.