Feature/dfi cleanup

[PengZheng]

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: 876471dd5f82ad676441a7340035cc110fbfcc6d 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.

[codecov-commenter]

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 :warning:
...te_service_admin_dfi/src/export_registration_dfi.c	50.00%	1 Missing :warning:

Additional details and impacted files

```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[PengZheng]

When I modified TEST_F(JsonSerializerTests, ParseTests) as following, a crash happened:

type = nullptr; 
inst = nullptr; 
rc = dynType_parseWithStr(example5_descriptor, nullptr, nullptr, &type); 
ASSERT_EQ(0, rc); 
rc = jsonSerializer_deserialize(type, example5_input, strlen(example5_input), &inst); 
ASSERT_EQ(0, rc);

json_auto_t* result = nullptr; 
// crash happens HERE! 
rc = jsonSerializer_serializeJson(type, inst, &result); 
ASSERT_EQ(0, rc);

I found two issues:

1. In example5_input, the node pointed to by head misses value member. Similarly, the node pointed to by head->left misses right member.
2. nullptr is not dealt in jsonSerializer_serializeJson, which leads to the crash.

        case '*' :
            subType = dynType_typedPointer_getTypedType(type);
            status = jsonSerializer_writeAny(subType, *(void **)input, &val); // dereference null ptr
            break;

For the first issue, IMO all struct members should be present in the json object, because C struct is very rigid, unlike json object. Our data model is based on C struct not on flexible json object.

For the second issue, I think nullptr should also be serialized/deserialized.

WDYT? @pnoltes

[pnoltes]

When I modified TEST_F(JsonSerializerTests, ParseTests) as following, a crash happened:

type = nullptr; 
inst = nullptr; 
rc = dynType_parseWithStr(example5_descriptor, nullptr, nullptr, &type); 
ASSERT_EQ(0, rc); 
rc = jsonSerializer_deserialize(type, example5_input, strlen(example5_input), &inst); 
ASSERT_EQ(0, rc);

json_auto_t* result = nullptr; 
// crash happens HERE! 
rc = jsonSerializer_serializeJson(type, inst, &result); 
ASSERT_EQ(0, rc);

I found two issues:

1. In example5_input, the node pointed to by head misses value member. Similarly, the node pointed to by head->left misses right member.
2. nullptr is not dealt in jsonSerializer_serializeJson, which leads to the crash.

        case '*' :
            subType = dynType_typedPointer_getTypedType(type);
            status = jsonSerializer_writeAny(subType, *(void **)input, &val); // dereference null ptr
            break;

For the first issue, IMO all struct members should be present in the json object, because C struct is very rigid, unlike json object. Our data model is based on C struct not on flexible json object.

For the second issue, I think nullptr should also be serialized/deserialized.

WDYT? @pnoltes

I agree with both. And making a nullptr possible is good improvement I had not considered.

Concerning JSON, If you treat the descriptor as a low level (JSON) schema, it is also logical that the expected JSON should have the expected members.

[PengZheng]

I agree with both. And making a nullptr possible is good improvement I had not considered. Concerning JSON, If you treat the descriptor as a low level (JSON) schema, it is also logical that the expected JSON should have the expected members.

These two issues have been addressed. I have questions regarding function argument meta info: what are the precise definitions of these argument types? This has been addressed.

For am=handle, it is clear that a handle should be untyped pointer, with descriptor 'P'. For am=pre, the Doxygen documentation says that the type should be pre-allocated by the function caller. Let me call these types "pre-allocatable". The following plain old C struct should qualify as pre-allocatable:

struct preallocatable {
    int a;
    int b;
}

Is text pre-allocatable? I guess not, since the caller can not predict how long a string will be.

struct not_sure {
    int a;
    int b;
    struct preallocatable* c;
}

Is not_sure pre-allocatable? On the service provider side, the callee could allocate a struct preallocatable and fill the c field. Then not_sure can be serialized successfully. However, on the service consumer side, it can caused a use-after-free crash. In jsonRpc_handleReply, we deal with pre-allocatable argument like following:

if (meta == DYN_FUNCTION_ARGUMENT_META__PRE_ALLOCATED_OUTPUT) {
                void* tmp = NULL;
                void** out = (void **) args[i];
                size_t size = 0;

                argType = dynType_typedPointer_getTypedType(argType);
                status = jsonSerializer_deserializeJson(argType, result, &tmp);
                if (tmp != NULL) {
                    size = dynType_size(argType);
                    memcpy(*out, tmp, size); // address of freed struct preallocatable is copied
                }

                dynType_free(argType, tmp); // preallocatable is freed
            }

If we do have a precise definition of pre-allocatability, such errors can be catched in dynFunction_parse:

        if (strcmp(meta, "handle") == 0) {
            if (dynType_descriptorType(real) != 'P') {
                celix_err_pushf("Error 'handle' is only allowed for untyped pointer not '%c'", dynType_descriptorType(real));
                return PARSE_ERROR;
            }
            arg->argumentMeta = DYN_FUNCTION_ARGUMENT_META__HANDLE;
        } else if (strcmp(meta, "pre") == 0) {
            if (!dynType_isPreallocatable(real)) {
                celix_err_pushf("Error 'am=pre' is only allowed for preallocatable type");
                return PARSE_ERROR;
            }
            arg->argumentMeta = DYN_FUNCTION_ARGUMENT_META__PRE_ALLOCATED_OUTPUT;
        }

Similar considerations also apply to other types. For example, for a standard argument to work, it should at least be serializable. For exmaple, int is serializable, int * is serailizable, while int** is not, because we can not tell from a json null whether int* == NULL or int** == NULL.

Due to the lack of precise definitions of these argument types, I am stuck in json_rpc.c. Though it is already 100% line covered, it is still very fragile IMHO. @pnoltes

[pnoltes]

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

[PengZheng]

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.