Snmalloc scratch #1

achamayou · 2020-05-27T08:24:08Z

@anakrish with these three tweaks commented inline (*), it's possible to build with -nostdinc, but:

You have to run ninja a bunch of times, because the install of ${PROJECT_BINARY_DIR}/output/include/openenclave/libc is typically in progress by the time you hit code that needs it. I'm trying to work out what cmake dependencies I need to mod for this, if you happen to know, please let me know!
I think the lack of address.h in malloc.cc is an oversight, I will fix that upstream (but having to do it in allocator.cpp is merely inelegant, not a blocker)

Pinging @mjp41, as usual :)

(*) unfortunately I've started from an up to date OpenEnclave, so that's picked out more changes than it should, mea culpa. I will inline comment the relevant changes, please ignore the rest. This isn't meant as an actual PR, just a way to show the changes.

The test result is OS-specific that log file has different line endings on Windows and other OS. Git always adjusts the line endings automatically. Thus this test has a prerequisite that git must execute on the same platform as on which platform the test runs. For example, git on cygwin changes line endings to lf and test on Windows expects crlf, then test will fail. This change make the libcxxrt test stateless. Signed-off-by: Alvin Chen <alvin@chen.asia>

3040: Fix libcxxrt test result on Windows. r=mingweishih a=nonpolarity The test result is OS-specific that log file has different line endings on Windows and other OS. Git always adjusts the line endings automatically. Thus this test has a prerequisite that git must execute on the same platform as on which platform the test runs. For example, git on cygwin changes line endings to lf and test on Windows expects crlf, then test will fail. This change make the libcxxrt test stateless. Signed-off-by: Alvin Chen <alvin@chen.asia> Co-authored-by: Alvin Chen <alvin@chen.asia>

Signed-off-by: yan xue <yan.xue@intel.com>

Signed-off-by: Anand Krishnamoorthi <anakrish@microsoft.com>

…AIN_PCK_PROC_CA. Signed-off-by: Qiucheng Wang <qiucwang@microsoft.com>

3018: Fix openenclave#2808, remove endorsement field CRL_ISSUER_CHAIN_PCK_PROC_CA. r=yentsanglee a=qiucwang Fixes openenclave#2808 Removed OE_SGX_ENDORSEMENT_FIELD_CRL_ISSUER_CHAIN_PCK_PROC_CA from endorsement struct. Current endorsement struct looks like: typedef enum _oe_sgx_endorsements_fields_t { OE_SGX_ENDORSEMENT_FIELD_VERSION, OE_SGX_ENDORSEMENT_FIELD_TCB_INFO, OE_SGX_ENDORSEMENT_FIELD_TCB_ISSUER_CHAIN, OE_SGX_ENDORSEMENT_FIELD_CRL_PCK_CERT, OE_SGX_ENDORSEMENT_FIELD_CRL_PCK_PROC_CA, **OE_SGX_ENDORSEMENT_FIELD_CRL_ISSUER_CHAIN_PCK_CERT, OE_SGX_ENDORSEMENT_FIELD_CRL_ISSUER_CHAIN_PCK_PROC_CA,** OE_SGX_ENDORSEMENT_FIELD_QE_ID_INFO, OE_SGX_ENDORSEMENT_FIELD_QE_ID_ISSUER_CHAIN, OE_SGX_ENDORSEMENT_FIELD_CREATION_DATETIME, OE_SGX_ENDORSEMENT_COUNT } oe_sgx_endorsements_fields_t; In `oe_create_sgx_endorsements()`, both fields are copied from `quote_verification_collateral->pck_crl_issuer_chain` so they have the identical content. https://github.com/openenclave/openenclave/blob/91d3069bf072addeab6151d1cfe8a39d1dd529be/common/sgx/endorsements.c#L65-L69 First `buffer` points to `ISSUER_CHAIN_PCK_CERT` and second one points to `ISSUER_CHAIN_PCK_PROC_CA` https://github.com/openenclave/openenclave/blob/91d3069bf072addeab6151d1cfe8a39d1dd529be/common/sgx/endorsements.c#L196-L212 And later in `oe_validate_revocation_list()`, both fields are wrapped into a `oe_cert_chain_t crl_issuer_chain[3]` https://github.com/openenclave/openenclave/blob/91d3069bf072addeab6151d1cfe8a39d1dd529be/common/sgx/collateral.c#L224 https://github.com/openenclave/openenclave/blob/91d3069bf072addeab6151d1cfe8a39d1dd529be/common/sgx/collateral.c#L283-L293 and used to verify the `PCK_CERT` by `oe_cert_verify()`. But in `oe_cert_verify()`, crl_issuer_chain is treated as `oe_cert_chain_t*` and only the first element, `ISSUER_CHAIN_PCK_CERT`, is used to verify the `cert`. And one issuer chain is enough to verify one certificate chain. https://github.com/openenclave/openenclave/blob/91d3069bf072addeab6151d1cfe8a39d1dd529be/include/openenclave/internal/crypto/cert.h#L126-L146 So `ISSUER_CHAIN_PCK_PROC_CA` is duplicated with `ISSUER_CHAIN_PCK_CERT` and also not used in verification. So we could remove it. Signed-off-by: Qiucheng Wang <qiucwang@microsoft.com> Co-authored-by: Qiucheng Wang <qiucwang@microsoft.com>

Allow all the regular ecalls to first finish during enclave initialization. Then launch ecall worker threads and wait for the threads to enter the enclave and thus acquire a dedicated tcs. This allows creating enclaves where all ecalls after initialization are switchless. Signed-off-by: Anand Krishnamoorthi <anakrish@microsoft.com>

3065: Switchless one tcs test from Intel and accompanying code fix r=anakrish a=anakrish Allow all the regular ecalls to first finish during enclave initialization. Then launch ecall worker threads and wait for the threads to enter the enclave and thus acquire a dedicated tcs. This allows creating enclaves where all ecalls after initialization are switchless. Intel test from openenclave#2887 Co-authored-by: yan xue <yan.xue@intel.com> Co-authored-by: Anand Krishnamoorthi <anakrish@microsoft.com>

- Docs: Add --recursive flag to git clone - Update Changelog - Use snmalloc only on SGX. The test is a basic test to ensure that snmalloc is compiled successfully as part of a build of the OE SDK. This test will be extended to make sure that only snmalloc symbols are present in the enclave. OPTEE does not support multi-threaded enclaves; and snmalloc currently has compile errors on ARM due to use of iostream Signed-off-by: Anand Krishnamoorthi <anakrish@microsoft.com>

- Call oe_allocator_init in td initialization rather than enclave initialization. This is because, the way the code is currently structures, td initialization happens before enclave initialization. - Add an explicit link to oe_allocator_malloc in link.c. This causes the first available definition of oe_allocator_malloc to be picked up. The user is expected to place the custom allocator library first in the linker line, ahead of oelibc. Signed-off-by: Anand Krishnamoorthi <anakrish@microsoft.com>

This change is temporarily proposed in order to unblock our Azure CI/CD infrastructure rebuild job. New infrastructure Azure Windows images built with PR openenclave#3051 fail the tests with: ``` [4/4105] Generating oeedger8r.exe FAILED: tools/oeedger8r/oeedger8r.exe Error copying file "C:/Users/oeadmin/workspace/pipelines/Azure-Windows/tools/oeedger8r/intel/*" to "C:/Users/oeadmin/workspace/pipelines/Azure-Windows/build/X64-Debug/tools/oeedger8r". ``` Reverting PR openenclave#3051 fixes this regression. Signed-off-by: Ionut Balutoiu <ibalutoiu@cloudbasesolutions.com>

3072: Revert PR openenclave#3051 r=BRMcLaren a=ionutbalutoiu This change is temporarily proposed in order to unblock our Azure CI/CD infrastructure rebuild job. New infrastructure Azure Windows images built with PR openenclave#3051 fail the tests with: ``` [4/4105] Generating oeedger8r.exe FAILED: tools/oeedger8r/oeedger8r.exe Error copying file "C:/Users/oeadmin/workspace/pipelines/Azure-Windows/tools/oeedger8r/intel/*" to "C:/Users/oeadmin/workspace/pipelines/Azure-Windows/build/X64-Debug/tools/oeedger8r". ``` Reverting PR openenclave#3051 fixes this regression. Signed-off-by: Ionut Balutoiu <ibalutoiu@cloudbasesolutions.com> Co-authored-by: Ionut Balutoiu <ibalutoiu@cloudbasesolutions.com>

Signed-off-by: brmclare <brmclare@microsoft.com>

3060: Bors Timout Time Increase r=BRMcLaren a=BRMcLaren We have been noticing Bors Timeouts. There is an upstream issue on the matter: https://forum.bors.tech/t/bors-keeps-timing-out/87/2. This PR Increase the Bors timeout to 12x the average ci run, allowing more time for runs to complete while others are queued before timing out. Co-authored-by: brmclare <brmclare@microsoft.com>

The Ocaml implementation has the following drawbacks - Ocaml is not that common a language and has proven to have a high barrier for entry to contributing to oeedger8r - Ocaml compiler/tools are not readily available on Windows This has led to - Using not actively maintained build on Ocaml on Windows - Or using esy which builds Ocaml compiler on each Windows machine it is run on, adding upto 30 additional minutes to building the OE SDK on Windows This C++ implementation is - intended to improve the ease of contribution to the oeedger8r by use of a common language that is well supported on all platforms - sticks to C++11 which is well supported - is based on a Python rewrite of oeedger8r https://github.com/openenclave/oeedger8r-python Signed-off-by: Anand Krishnamoorthi <anakrish@microsoft.com>

3069: EXPERIMENTAL: Use C++ implementation of oeedger8r r=anakrish a=anakrish Pass USE_CPP_EDGER8R=on to use C++ oeedger8r instead of Ocaml edger8r. C++ oeedger8r repository: https://github.com/openenclave/oeedger8r-cpp The Ocaml implementation has the following drawbacks - Ocaml is not that common a language and has proven to have a high barrier for entry to contributing to oeedger8r - Ocaml compiler/tools are not readily available on Windows This has led to - Using not actively maintained build on Ocaml on Windows - Or using esy which builds Ocaml compiler on each Windows machine it is run on, adding upto 30 additional minutes to building the OE SDK on Windows This C++ implementation is - intended to improve the ease of contribution to the oeedger8r by use of a common language that is well supported on all platforms - sticks to C++11 which is well supported - is based on a Python rewrite of oeedger8r https://github.com/openenclave/oeedger8r-python Signed-off-by: Anand Krishnamoorthi <anakrish@microsoft.com> Co-authored-by: Anand Krishnamoorthi <anakrish@microsoft.com>

3rdparty/snmalloc/CMakeLists.txt

3rdparty/snmalloc/allocator.cpp

achamayou · 2020-05-27T08:28:14Z

3rdparty/snmalloc/allocator.cpp

 #define USE_RESERVE_MULTIPLE 1
 #define IS_ADDRESS_SPACE_CONSTRAINED
 #define SNMALLOC_EXTERNAL_THREAD_ALLOC
 #define SNMALLOC_NAME_MANGLE(a) oe_allocator_##a

 // Enable the Open Enclave PAL(Platform Abstraction Layer) for Open Enclave,
 // see pal_open_enclave.h for details
+#include "./snmalloc/src/ds/address.h"


This should be included by malloc.cc, will fix upstream.

The PalOpenEnclave file should include address.h

microsoft/snmalloc#205

@mjp41 It seems address.h ought to be included by specifying the relative path from pal_openenclave.h. The compiler could not find "ds/address.h". I worked around this by adding snmalloc/src as an include directory when building snmalloc for OE.

anakrish · 2020-05-30T01:21:23Z

Thanks for the PR @achamayou, I have incorporated changed into openenclave#3063.
Also expanded the test there to make sure dlmalloc is completely replaced when using snmalloc.

anakrish · 2020-05-30T01:23:31Z

1. You have to run ninja a bunch of times, because the install of ${PROJECT_BINARY_DIR}/output/include/openenclave/libc is typically in progress by the time you hit code that needs it. I'm trying to work out what cmake dependencies I need to mod for this, if you happen to know, please let me know!

I've fixed this in my PR.

anakrish · 2020-06-04T17:51:46Z

1. You have to run ninja a bunch of times, because the install of ${PROJECT_BINARY_DIR}/output/include/openenclave/libc is typically in progress by the time you hit code that needs it. I'm trying to work out what cmake dependencies I need to mod for this, if you happen to know, please let me know!

I've fixed this in my PR.

Turned out my fix didn't work and I couldn't reproduce it easily other than in CI.
After multiple attempts and experiments and failures, I finally figured out a workaround that works :)
I clearly looks like a bug with cmake/ninja in that dependent targets are launched even before an external project is successfully built.

The workaround involves adding a custom command that copied the output of the external project and having snmalloc have a source level dependency on the output of the custom command.
Quite hacky; but this is the only workaround that seems to work
https://github.com/openenclave/openenclave/blob/582b4dcbdb49d32b132454c2be0be480bc76c59c/3rdparty/snmalloc/CMakeLists.txt#L6-L25

# oesnmalloc depends on musl_includes and libcxx_includes.
# However specifying those external projects as dependencies does not seem to
# work. Build of oesnmalloc is launched while musl_includes is being built.
# This likely a cmake/ninja bug. Depending on oelibc_includes (which is an
# INTERFACE library that depends on musl_includes) also does not work.
# To ensure that musl_includes and libcxx_includes external projects are
# completed before oesnmalloc is built, we use the custom command below
# that depends on the external projects. The custom command copies header
# files generated by the external projects to oesnmalloc's binary directory.
# oesnmalloc lists the copied header files as sources.
add_custom_command(
  OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/deprecations.h
         ${CMAKE_CURRENT_BINARY_DIR}/__config.h
  COMMAND
    ${CMAKE_COMMAND} -E copy
    ${PROJECT_BINARY_DIR}/output/include/openenclave/libc/bits/deprecations.h
    ${CMAKE_CURRENT_BINARY_DIR}/deprecations.h
  COMMAND ${CMAKE_COMMAND} -E copy ${LIBCXX_INCLUDES}/__config
          ${CMAKE_CURRENT_BINARY_DIR}/__config.h
  DEPENDS musl_includes libcxx_includes)

add_enclave_library(
  oesnmalloc
  OBJECT
  allocator.cpp
  # List the following as sources of oesnmalloc in order to ensure that
  # the musl_includes and libcxx includes complete before oesnmalloc build is
  # started.
  deprecations.h
  __config.h)

anakrish · 2020-06-04T17:53:20Z

Thanks a lot for this PR @achamayou
openenclave#3063 has been merged and snmalloc is now built and tested in CI.

achamayou · 2020-06-04T21:35:38Z

@anakrish that cmake behaviour is very odd, it’s unfortunate that defining the intermediary command is necessary. Thank you for working it out though!

4055: Cleaning up ocall_buffer allocated in _setup_ecall_context r=anakrish a=mrragava Fixing a leak scenario detected during fuzzing. Signed-off-by: Ragavan Dasarathan <mrragava@gmail.com> ==9977==ERROR: LeakSanitizer: detected memory leaks Direct leak of 16384 byte(s) in 1 object(s) allocated from: #0 0x551523 in __interceptor_malloc /home/ragava/Desktop/labs/llvm/oe-llvm/compiler-rt/lib/asan/asan_malloc_linux.cc:146:3 #1 0x83cddb in _setup_ecall_context /home/ragava/oefuzz/openenclave-security/sut/openenclave/host/sgx/enter.c:143:33 #2 0x83cddb in __morestack /home/ragava/oefuzz/openenclave-security/sut/openenclave/host/sgx/enter.c:202 #3 0x82d120 in _do_eenter /home/ragava/oefuzz/openenclave-security/sut/openenclave/host/sgx/calls.c:193:13 #4 0x82d120 in oe_ecall /home/ragava/oefuzz/openenclave-security/sut/openenclave/host/sgx/calls.c:612 #5 0x66328d in _initialize_enclave /home/ragava/oefuzz/openenclave-security/sut/openenclave/host/sgx/create.c:464:5 #6 0x660fe9 in oe_create_enclave /home/ragava/oefuzz/openenclave-security/sut/openenclave/host/sgx/create.c:1190:5 #7 0x59c8e5 in oe_create_hostapis_enclave /home/ragava/oefuzz/openenclave-security/build/fuzzing_build/src/dynamic/fuzzing/hostapis/host/hostapis_u.c:3925:12 #8 0x583431 in hostapi_fuzzer::hostapi_fuzzer() /home/ragava/oefuzz/openenclave-security/build/fuzzing_build/../../src/dynamic/fuzzing/hostapis/host/host.cpp:58:13 #9 0x582bea in parse_report_fuzz::parse_report_fuzz() /home/ragava/oefuzz/openenclave-security/build/fuzzing_build/../../src/dynamic/fuzzing/hostapis/host/host.cpp:119:7 #10 0x581e2f in std::_MakeUniq<parse_report_fuzz>::__single_object std::make_unique<parse_report_fuzz>() /usr/lib/gcc/x86_64-linux-gnu/7.5.0/../../../../include/c++/7.5.0/bits/unique_ptr.h:821:34 #11 0x581bb1 in LLVMFuzzerTestOneInput /home/ragava/oefuzz/openenclave-security/build/fuzzing_build/../../src/dynamic/fuzzing/hostapis/host/host.cpp:213:18 ... ... Co-authored-by: Ragavan Dasarathan <mrragava@gmail.com>

4520: Fix bors builds r=CyanDevs a=CyanDevs Multibranch pipelines (which Bors runs as) behave a bit differently than manually triggered pipelines in terms of parameters. 1. It does not specify user-defined parameters. If there are any user-defined parameters, then use the defaults. E.g. `params.BRANCH_NAME` or `$BRANCH_NAME` in shell would be master. 2. To differentiate between different branch in a multibranch pipeline job, an environment variable `BRANCH_NAME` is used and is normally referred to as `env.BRANCH_NAME` or `$BRANCH_NAME` in shell. The problem is both user-defined parameters and Groovy variables are loaded into environment as environment variables (thanks to workflow-cps-plugin) and there is an order of precedence. Point #1 became relevant as user-defined parameters were added to the declarative pipeline in [my last PR](https://github.com/openenclave/openenclave/pull/4505/files#diff-917491029f94ac1ce6c114a542d2a8738ce1f2d1e866eaffe7189d2139250200)). Specifically the user-defined parameter param.BRANCH_NAME took precedence over Groovy variable BRANCH_NAME when loaded into the environment. Therefore both env.BRANCH_NAME, params.BRANCH_NAME, and BRANCH_NAME all referred to the same variable and led to Bors always checking out `master`. This PR fixes the Bors builds by renaming conflicting parameter name, and prefixing global Groovy variables with `GLOBAL_` to be more explicit. So `GLOBAL_BRANCH_NAME` will be set by `env.BRANCH_NAME` (point #2), or if that does not exist, the user-defined parameter `params.BRANCH` (point #1). As a side issue, this also changes `REPOSITORY_NAME`, `DOCKER_TAG` and `FULL_TEST_SUITE` to solely a user-defined parameter. Signed-off-by: Chris Yan <chrisyan@microsoft.com> Co-authored-by: Chris Yan <chrisyan@microsoft.com>

nonpolarity and others added 18 commits May 15, 2020 11:48

add one tcs test

554d97a

Signed-off-by: yan xue <yan.xue@intel.com>

Use oehost instead oehostapp

d67a9ac

Signed-off-by: Anand Krishnamoorthi <anakrish@microsoft.com>

Fixed openenclave#2808, remove OE_SGX_ENDORSEMENT_FIELD_CRL_ISSUER_CH…

f94c6c4

…AIN_PCK_PROC_CA. Signed-off-by: Qiucheng Wang <qiucwang@microsoft.com>

bors timout increase

65b16b9

Signed-off-by: brmclare <brmclare@microsoft.com>

Merge remote-tracking branch 'anakrish/ak_snmalloc_update'

7e63fd5

nostdinc

67e52e3

achamayou requested a review from anakrish as a code owner May 27, 2020 08:24

achamayou commented May 27, 2020

View reviewed changes

3rdparty/snmalloc/CMakeLists.txt Show resolved Hide resolved

achamayou commented May 27, 2020

View reviewed changes

3rdparty/snmalloc/allocator.cpp Show resolved Hide resolved

achamayou commented May 27, 2020

View reviewed changes

achamayou mentioned this pull request May 27, 2020

OpenEnclave: Compilation fails with -nostdinc due to cpuid.h inclusion microsoft/snmalloc#200

Closed

anakrish closed this Jun 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Snmalloc scratch #1

Snmalloc scratch #1

achamayou commented May 27, 2020 •

edited

Loading

achamayou May 27, 2020

mjp41 May 27, 2020

mjp41 May 27, 2020

anakrish May 30, 2020

anakrish commented May 30, 2020

anakrish commented May 30, 2020

anakrish commented Jun 4, 2020

anakrish commented Jun 4, 2020

achamayou commented Jun 4, 2020

Snmalloc scratch #1

Snmalloc scratch #1

Conversation

achamayou commented May 27, 2020 • edited Loading

achamayou May 27, 2020

Choose a reason for hiding this comment

mjp41 May 27, 2020

Choose a reason for hiding this comment

mjp41 May 27, 2020

Choose a reason for hiding this comment

anakrish May 30, 2020

Choose a reason for hiding this comment

anakrish commented May 30, 2020

anakrish commented May 30, 2020

anakrish commented Jun 4, 2020

anakrish commented Jun 4, 2020

achamayou commented Jun 4, 2020

achamayou commented May 27, 2020 •

edited

Loading