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

Feature/dfi cleanup #699

Merged
merged 71 commits into from
Feb 2, 2024
Merged

Feature/dfi cleanup #699

merged 71 commits into from
Feb 2, 2024

Conversation

PengZheng
Copy link
Contributor

@PengZheng PengZheng commented Dec 14, 2023

Introduction

This PR is a preliminary work of #590. The main motivation behind this is to familiarize myself of the underlying mechanism of RSA, i.e. libdfi. Reading the code base alone is not enough, it should also be thoroughly tested and debugged. That way not only did I get a full understanding, I also found and fixed several ambiguities and bugs along the way.

Hopefully, after this PR merged, libdfi is robust enough to handle most (if not all) of malformed descriptors and all malicious JSON requests/responses.

Precise Definitions of DFI Argument Types

Dyn function argument meta (am) as meta info, with the following possible values
am=handle #void pointer for the handle
am=pre #output pointer with memory pre-allocated, it should be pointer to trivial types, check dynType_isTrivial for more info.
am=out #output pointer, it should be pointers to text or pointer to serializable point type
Without meta info the argument is considered to be a standard argument, which can be of any serializable type.

Previously, we don't have such "formal" definitions. As mentioned by #723, it is fairly easy to construct "legal" interface descriptor to introduce use-after-free bugs. To address this, we introduce the notion of trivial type and perform strict checking in dynFunction_parse. Note that "serializability" check is left for the jsonSerializer to perform.

RSA Interface Convention Enforcement

  • The first parameter of a method must be handle.
  • am=handle can appear exactly once.
  • If exists, output parameter (either am=pre or am=out) is only allowed as the last one. Therefore, there is at most one output parameter.

We enforce the convention in dynInterface_checkInterface so that arguments handling in json_rpc.c can be greatly simplified.

Please consider adding the above two into #690. @xuzhenbao

Early Return Error Handling Pattern

Previously, error handling is done using chained status check:

    status = do_something();
    if (status == OK) {
        status = do_something_else();
    }
    if (status == OK) {
        status = do_other_thing();
    }

With chained status check, it is relative easy to achieve high line coverage.
However, the branch coverage is still low and the control flow is often difficult to follow.
By applying early return uniformly in libdfi, by archiving high line coverage, we are almost guaranteed high branch coverage and the readability is often improved.

Remove AVRO

It is currently incomplete and unused, so is removed by this PR.
After completing #590, we can reconsider introducing more efficient serialization mechanism (including AVRO).

Other Enhancements and Improvements

  1. We can now serialize/deserialize nullptr for pointer type.
  2. Double pointer is not serializable. For exmaple, we can not tell from a json null whether int* == NULL or int** == NULL.
  3. celix_auto is used extensively. Please note that a caveat related to stack variable declaration order is highlighted in this commit: 876471d The interesting thing is that this issue only manifests itself in clang builds.
  4. const qualifier is applied where appropriate.
  5. Add dynType_realType to deal with reference type and fix several related crashes.

`tc.cache_variables["CMAKE_EXE_LINKER_FLAGS"] = "-Wl,--unresolved-symbols=ignore-in-shared-libs"` would be overriden previously.
@codecov-commenter
Copy link

codecov-commenter commented Dec 14, 2023

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (29fa7d0) 88.50% compared to head (90d643a) 88.90%.

Files Patch % Lines
...te_service_admin_dfi/src/import_registration_dfi.c 52.63% 9 Missing ⚠️
...te_service_admin_dfi/src/export_registration_dfi.c 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #699      +/-   ##
==========================================
+ Coverage   88.50%   88.90%   +0.40%     
==========================================
  Files         212      216       +4     
  Lines       24606    24293     -313     
==========================================
- Hits        21777    21598     -179     
+ Misses       2829     2695     -134     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@PengZheng PengZheng changed the title Feature/dfi cleanup [WIP]Feature/dfi cleanup Dec 14, 2023
Also remove dync_common from public interface of dfi.
…improve interface ergonomic to dyn_interface.
# Conflicts:
#	bundles/pubsub/pubsub_serializer_json/src/pubsub_json_serialization_provider.c
#	bundles/pubsub/pubsub_utils/gtest/src/PubSubMatchingTestSuite.cpp
#	bundles/pubsub/pubsub_utils/gtest/src/PubSubSerializationHandlerTestSuite.cc
#	bundles/pubsub/pubsub_utils/src/pubsub_serialization_provider.c
…n_descriptor and improve ergonomics of dyn_message API.
# Conflicts:
#	libs/error_injector/stdio/CMakeLists.txt
#	libs/error_injector/stdio/include/stdio_ei.h
#	libs/error_injector/stdio/src/stdio_ei.cc
1. The first method argument must be of handle type.
2. Only one output argument (either am=pre or am=out) is allowed.
3. Output argument (if any) must be the last argument.
1. Apply early return to make error handling dead simple.
2. Remove unnecessary argument list iterations.
3. Protect against user-provided nullptr.
4. Deal with invalid text output.
…e jsonRpc_prepareInvokeRequest.

1. Remove unnecessary argument list iteration by removing usages of dynFunction_argumentTypeForIndex and dynFunction_argumentMetaForIndex.
2. Add more tests using error injection.
1. Add support for nullptr result for `am=out` parameter.
2. Fix memory leaks when `am=out` result fails to serialize.
3. Extract dynInterface_findMethod.
For any method, there must be exactly one handle argument, that is, the first one.
1. Limit max number of arguments to CELIX_JSON_RPC_MAX_ARGS.
2. Apply early return error handling.
3. Simplify arguments manipulation and check for argument number mismatch.
4. More error injection tests.
`rpcArgs` refers to stack variables, which must be declared before `rpcArgs`.
@PengZheng PengZheng changed the title [WIP]Feature/dfi cleanup Feature/dfi cleanup Jan 28, 2024
@pnoltes
Copy link
Contributor

pnoltes commented Jan 28, 2024

I will try to find to time to review this PR, this week.

Copy link
Contributor

@pnoltes pnoltes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still reviewing.

Nice to see that libdfi gets some attention and refactoring :)
I really enjoy seeing te early returns and usage of celix_err.

It has been a while since I looked into this code, so reviewing will take some days.
I do remember that I really liked creating libdfi; It is quite a powerful concept, complex to implement and wrap your head around (ffi with a pointer to a argument array, where the argument entries are pointers to the argument values and the argument values can be pointers or even a double output pointers) , but it possible to nice split up the parsing and usage in small functions.

Originally my idea was also to allow optional usage of libdfi in the framework so that it would be possible to (e.g.) use a service struct of version 1.0.0 while the provided service is 1.1.0. With libdfi is should be possible to ad-hoc create a 1.0.0 version based on the 1.0.0 and 1.1.0 descriptor (assuming correct usage of semver). But I digress.

libs/dfi/include/dyn_type.h Outdated Show resolved Hide resolved
libs/dfi/include/dyn_interface.h Outdated Show resolved Hide resolved
libs/dfi/src/dyn_common.c Show resolved Hide resolved
libs/dfi/src/dyn_common.c Outdated Show resolved Hide resolved
libs/dfi/src/dyn_common.c Outdated Show resolved Hide resolved
libs/dfi/src/dyn_type.c Outdated Show resolved Hide resolved
libs/dfi/src/dyn_type.c Show resolved Hide resolved
Comment on lines +266 to +267
if (!dynType_isTrivial(entry->type)) {
type->trivial = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!dynType_isTrivial(entry->type)) {
type->trivial = false;
type->trivial = dynType_isTrivial(entry->type);

Copy link
Contributor Author

@PengZheng PengZheng Jan 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention here is if ANY of its field is non-trivial, then the complex itself is non-trivial, otherwise it is trivial (as set in line 247).

But with the suggested modification, the triviality of a complex is determined by its last field.

libs/dfi/src/dyn_type.c Show resolved Hide resolved
libs/dfi/src/dyn_type.c Show resolved Hide resolved
@PengZheng
Copy link
Contributor Author

PengZheng commented Jan 31, 2024

It has been a while since I looked into this code, so reviewing will take some days.
I do remember that I really liked creating libdfi; It is quite a powerful concept, complex to implement and wrap your head around (ffi with a pointer to a argument array, where the argument entries are pointers to the argument values and the argument values can be pointers or even a double output pointers) , but it possible to nice split up the parsing and usage in small functions.

Originally my idea was also to allow optional usage of libdfi in the framework so that it would be possible to (e.g.) use a service struct of version 1.0.0 while the provided service is 1.1.0. With libdfi is should be possible to ad-hoc create a 1.0.0 version based on the 1.0.0 and 1.1.0 descriptor (assuming correct usage of semver). But I digress.

Thanks for sharing the background with me. I pretended to be cool, but let me admit it: the libdfi work is eyeopening, which leads to another "WOW, things could really be done like this" awe.

Inspired by the Rust POC, to be more concrete, the build.rs and procedural macro, I think clang/libclang integrated with the build system is worth trying for framework side code generation/transformation. RSA descriptor is an ideal code generation target.

Copy link
Contributor

@pnoltes pnoltes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Nice and needed refactoring libdfi.

@PengZheng PengZheng merged commit ce135f5 into master Feb 2, 2024
32 checks passed
@PengZheng PengZheng deleted the feature/dfi-cleanup branch February 2, 2024 14:27
@PengZheng PengZheng mentioned this pull request Feb 22, 2024
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

Successfully merging this pull request may close these issues.

Precise Definitions of DFI Argument Types
3 participants