duktape icon indicating copy to clipboard operation
duktape copied to clipboard

Add basic Intel SGX support

Open hf opened this issue 6 years ago • 18 comments

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:

  1. 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.
  2. 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 from sys/cdefs.h which 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 define INTELSGX manually or use --platform=intelsgx in the configure.py invocations.
  3. 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_NOW and DUK_USE_DATE_GET_LOCAL_TZOFFSET. These are mandatory and well documented, and compilation fails with a preprocessor error if they are not added.
  4. 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_SPRINTF needed to be refactored into DUK_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.
  5. Add DUK_USE_STANDARDIZED_POINTER_ENCODING option (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 functions duk_encode_pointer_cstr and duk_decode_pointer_cstr were added to duk_util_misc.c. They have also been replaced wherever the %p format specifier was used directly or indirectly (except for debug statements, of course). By enabling this option, Duktape no longer uses DUK_SSCANF.
  6. Added a few things to .gitignore, most importantly runtests/package-lock.json which is added by npm when tests are run locally. It may be a good idea to remove this from .gitignore if we want to pin down versions.
  7. Changed invocation of python within runtests/runtests.js to python2.7 since running the ecmatest with Python 3 failed with error. These days Python 3 is behind python, while 2.7 can be invoked with python2.7.

This PR is not complete I'm still testing and changing things, but discussions can begin IMO.

Remaining things to complete:

  • apitest fails when DUK_USE_STANDARDIZED_POINTER_ENCODING because of the hardcoded expectation for 0xdeadbeef. ecmatest succeeds, tho. Possible solutions:
    1. Test expectations get passed through jade or some other preprocessor so they can be configured before being run.
    2. All 0xdeadbeef occurrences in expectations within tests/api are replaced with a configurable value.
    3. Change the standardized format, though I really like the simplicity of 1:1 memory layout in hex.
  • Add new doc page for how to compile for Intel SGX.
  • Write instructions for testing.

hf avatar Jan 07 '20 21:01 hf

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.

hf avatar Jan 08 '20 22:01 hf

Here are some possible solutions to the problem:

  1. Change reading and formatting pointers to functions for that purpose:
    #define DUK_PTR2STR(buf,sz,ptr) DUK_SNPRINTF(buf,sz,"%p",ptr)
    #define DUK_STR2PTR(buf,sz,ptr) DUK_SSCANF(buf,"%p",ptr)
    
    Then these can be set to different values for Intel SGX or other constrained environments.
  2. Knowingly leave the DUK_SSCANF use in DUK_USE_JX sections, but add documentation and example configuration that disables DUK_USE_JX for Intel SGX.
  3. Define platform independent encoding of pointers without the use of printf and scanf families.

hf avatar Jan 08 '20 23:01 hf

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.

svaarala avatar Jan 11 '20 16:01 svaarala

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.

hf avatar Jan 11 '20 23:01 hf

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.

svaarala avatar Jan 22 '20 13:01 svaarala

I'll try to review the diff tomorrow night, assuming it is ready for comments.

svaarala avatar Jan 22 '20 13:01 svaarala

Yes, ready for comments @svaarala.

I agree on the point for DUK_F_INTELSGX and will produce changes toward that approach this weekend.

hf avatar Jan 24 '20 17:01 hf

@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 avatar Jan 31 '20 21:01 svaarala

@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.

hf avatar Feb 01 '20 11:02 hf

What's the ideal style for void**? :sweat_smile: Is it void* *?

hf avatar Feb 04 '20 22:02 hf

Still many broken indents. Something is wrong with my vim, it's confused by this codebase.

hf avatar Feb 05 '20 23:02 hf

What's the ideal style for void**?

In the current codebase it would simply be void **.

sva-p avatar Feb 06 '20 06:02 sva-p

@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.

svaarala avatar Feb 12 '20 14:02 svaarala

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. :)

hf avatar Feb 12 '20 19:02 hf

Adding a .editorconfig sounds good to me. I'll steer clear of debating tabs vs spaces ;-)

svaarala avatar Feb 12 '20 20:02 svaarala

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:

  1. https://github.com/svaarala/duktape/pull/2219#discussion_r373690188

hf avatar Feb 29 '20 12:02 hf

I'll take a look - after that commits should be squashed for merging (a single commit is fine).

svaarala avatar Feb 29 '20 12:02 svaarala

@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.

svaarala avatar Mar 14 '20 21:03 svaarala