PengZheng commented on PR #699:
URL: https://github.com/apache/celix/pull/699#issuecomment-1905582200
> 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?
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:
```C
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.
```C
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:
```C
if (meta == DYN_FUNCTION_ARGUMENT_META__PRE_ALLOCATED_OUTPUT) {
void* tmp = NULL;
void** out = (void **) args[i];
size_t size = 0;
if (dynType_descriptorType(argType) == 't') {
status = jsonSerializer_deserializeJson(argType, result,
&tmp);
if (tmp != NULL) {
size = strnlen(((char *) *(char**) tmp), 1024 *
1024);
memcpy(*out, *(void**) tmp, size);
}
} else {
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`:
```C
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@celix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
