Hello again,

thanks for all the recent patches and improvements.

RE: c89 support https://github.com/camgunz/cmp/issues/36

How can we prevent this (and other compiler gotchas) in the future? I don't know your workflow but I can guess you might be a busy guy.

I propose setting up https://travis-ci.org for this project that that will compile camgunz/cmp in a few ways (-std=c89 -Wal -Wextra -Werr) and maybe with Clang and it's static analyzer and beep if warnings occur.

I can set this up as a fork+PR but in the end you'll have to be comfortable with travis-ci.org having access to your repo.

Regards,

n

So #3 proposed adding skipping to CMP, and there's some discussion there.

After trying to implement a version, it looks like the only way to fully implement this is by creating a SAX-style state machine.

"WHY!?", you might ask. Well I'll tell you, hopefully I'm wrong.

It's not a backend-support problem. We can add an optional skip callback, and CMP can just set an error whenever cmp_skip_object is called on a context where that callback is NULL.

The problem is nested arrays and maps. The naive approach is to just have cmp_skip_object recursively call itself, but that leaves CMP open to stack overflow attacks via specifically-crafted data. I absolutely will not do that.

The alternative is to have a bunch of state in cmp_ctx_t itself and use the heap. There are a few downsides to this:

• It makes heap allocations, and it's doing this for every nested array/map. This is, therefore, a vector for memory exhaustion given specifically-crafted data.
• It's a complicated little state machine, in contrast with the rest of CMP, which is extremely clear.
• It would cause CMP to depend on the C Standard Library, and thus would require a build system. Not that that's a big deal, but it's still a definite paradigm shift.

I vote against adding skipping to CMP. To use the example in #3 of an RPC server, let's say you're getting MessagePack data as a stream and that's your CMP backend (error handling omitted):

uint32_t map_size;

cmp_read_map(&cmp, &map_size)

for (uint32_t i = 0; i < map_size; i++) {
    char *arg_name;
    uint32_t str_size;

    cmp_read_str_size(&cmp, &str_size);
    read_netstream_string(&arg_name, str_size);

    if (!rpc_arg_is_valid(arg_name)) {
        cmp_read_str_size(&cmp, &str_size);
        skip_netstream_bytes(str_size);
    }
}

This is pretty simple, and the only thing that would be different if CMP added cmp_skip_next_object is skip_netstream_bytes(str_size) is replaced with cmp_skip_next_object(&cmp).

The problem is that cmp_skip_next_object might have to skip a map containing 5000 other arrays, each containing 5000 arrays, each containing 5000 arrays that each contain 5000 entries of the Gettysburg Address. Skipping an unwanted string is much simpler than skipping the next object, whatever it might be. Furthermore, skipping can most easily be handled using backend API's designed specifically for that; CMP can add no value there. Therefore, I think adding skipping to CMP is out of scope.

That said, I'm always open to arguments! :) If this is a feature you're really needing and you've got a cool idea on how to do it, I'm absolutely happy to work on it (or, even better, merge a PR ;) ). I just think it's not feasible.

Yes... thats the issue, ok, let me explain: The library is fully fulfilling the new spec, while most other implementations (Perl, JavaScript, to be specific) just support the old types. Which is something you dont notice AS LONG you never have a STR8 ... cause thats literally the only case where the stuff crash (And there are is no BIN support in the old spec, but that is not those implementations use it at all).

There is actually a very easy simple trick here to fix that, the library just needs to ignore STR8 for the packing and directly make STR16 instead. (2 comments, a define?) Out of the fact that I think many people will run into this problem if they mix the implementations (its REALLY not an obvious problem, took me 1-2 days to find out), i think it would be awesome if by default that would be part of the library (I can make the pull request, i mean its trivial), the question would only be how in my eyes (with define or different), but i think its crucial that the default behaviour just fits into "both ways".... the people still can enforce STR8 and they can still unpack STR8, there is no damage just the killing of problems of the bad state of the msgpack implementations.

What you think?

(P.S.: I am working on fixing the situation more general, but its hard to find responsible people ;), this is a first approach that would at least "fix" the situation)

First thank you for the excellent library :)

In cmp.c there are a lot of warnings reported by our analysis tool. See screenshot. The line numbers may not match, but the function names are mentioned.

Can you please evaluate if there is any problem with these implicit conversions? It would also be nice if you can cast accordingly to show your intention.

Hi,

I'm new to MsgPack and C, but I'm loving your implementation so far. I already have some great results. I'm using it to unpack / pack data from a network socket, which stores it into an unsigned char buffer. I couldn't get cmp to read / write directly on the char buffer instead of a file (your example). Is there a simple way to do that and if yes, could you provide me with a simple example how to do it? Thanks a lot in advance!

In my own code it would be very convenient if I could "peek" at an object before committing to reading it. What I am imagining is that each cmp_read__() function from the main api would have a corresponding peek method that returns true/false if the corresponding cmp_read__() function would, in principle, be successful.

if (cmp_peek_str(cmp)) { // It's a string, read it with cmp_read_str } else if (cmp_peek_int(cmp)) { // It's an int, read it with cmp_read_int } else if (cmp_peek_double(cmp)) { // etc.. } else { // error - not supported }

While you can accomplish something similar with cmp_read_object() by looking at the type of the cmp_object, you can't then take advantage of the logic already embedded in the cmp_read__() functions to know how the CMP_TYPE__ enumerations relate to each other and should be handled.

The duty of error checking would still fall mostly on the read functions but you could also add a cmp_peek_error() method that returns true/false if nothing can be peeked because of an error.

We would like to use the CMP library for kernel->user mode communication. But it is not allowed to use floating point instructions in the kernel. Therefore we have to exclude everything FPU related from code during compilation.

This is another attempt to add skipping support (see also #5). This implementation is based on code from CWPack and does not use recursion.

Before this is merged, two things should be considered:

1. obj_skip_count is currently a uint32_t and could overflow (e.g. while reading malicious data). Extending the type to uint64_t would help with the problem (at least a bit), but I didn't really want to change it, since I use cmp on an embedded device. Maybe instead, we could check for overflow and exit with an error.
2. Skipping of strings/bin/ext currently reads one byte at a time, since there is no buffer available. This could be replaced with a skip/seek function pointer. Instead a buffer of configurable size could be added (see #3).

When developing an application communicating between Python and C I ran into a problem with floating point numbers: python doesn't distinguish between simple and double precision. I figured it made sense to allow cmp to cast them transparently on read.

Hello, cmp_write_ext8_marker (and possibly others, as it is the only one I checked) have a slight error. They write the extension type before the data size, where the MessagePack 5 specification says :

ext 8 stores an integer and a byte array whose length is upto (2^8)-1 bytes:
+--------+--------+--------+========+
|  0xc7  |XXXXXXXX|  type  |  data  |
+--------+--------+--------+========+

where
* XXXXXXXX is a 8-bit unsigned integer which represents N

Can you fix it, or do you prefer if I do it myself ? Also, I think using some kind of unit testing for CMP would be awesome.

thank you for this very useful library by the way.

The function cmp_skip_object_limit() is documented to have its limit be a limit of depth, but it doesn't work properly when you have nested arrays or maps.

For example, if one had an array of size 10 that contained 10 zero-length arrays within, that should be fine with a limit of depth 2 according to the description. However, because of the way the code is written, each time an array is encountered, the depth is increased, regardless of whether it was nested or not. As a concrete example, a diff for test/test.c is attached. If you apply that, build, and run ./cmptest, the second bit of tests added fails, displaying this behavior.

Given that cmp.c is written without any heap allocations, I'm not sure what the solution is for tracking the depth here. One could add an arbitrarily-sized array to keep track of some data, but perhaps you can think of a more appropriate solution. Let me know if you need any more info on the specific issue!

After reading through the code (and using it in a pet project -- great work! Thanks!!), I have come across a situation where I need some way to skip over nested objects to find specific keys in a map. I have been unpacking into a nested map and then walking the map, but that adds a frustrating amount of complexity to my case, when I'd rather just use the msgpack binary blob as the memory backing the data structure.

You have in your comments that there is no way to do nested skips without allocations, but couldn't you just add a counter into cmp_ctx_t that tracks some limited amount of nesting (configurable at compile-time)?

#define CMP_RECURSION_DEPTH 4
#define CMP_RECURSION_DEPTH_BYTES 1

uint8_t recursion_count[CMP_RECURSION_DEPTH * CMP_RECURSION_DEPTH_BYTES];

Then each recursion can use CMP_RECURSION_DEPTH_BYTES to count up to the number of elements per recursion, and CMP_RECURSION_DEPTH can count up to the number of recursions supported. Obviously, this would be easier with allocations, but I completely understand your reluctance to add that.

Alternatively, you could handle it at runtime by adding

  uint8_t recursion_max; // set by cmp_ctx_init_with_recursion (new function)
  uint8_t recursion_stack[0]; // allocated along with the cmp_ctx_t struct

to the cmp_ctx_t struct. I think I'm over-complicating your very nice library, though.

If this is something you think is worthwhile, I would be happy to work on it and put in a PR for review. If not, I will likely still work on it and just keep the code in my own project. Thoughts?

The read/write functions are not symmetric and confusing:

bool cmp_write_integer(cmp_ctx_t *ctx, int64_t d);

bool cmp_read_int(cmp_ctx_t *ctx, int32_t *i);
bool cmp_read_long(cmp_ctx_t *ctx, int64_t *u);
bool cmp_read_integer(cmp_ctx_t *ctx, int64_t *u);

I would suggest the following:

• remove int/integer duplicate functions
• uses distinct names for 32-bit and 64-bit types: int/long or maybe int32/int64
• add dedicated 32-bit write function (read function is already there) -> this is especially advantageous for 32-bit systems, e.g. microcontrollers, where 64-bit ints are emulated in software

bool cmp_write_int(cmp_ctx_t *ctx, int32_t d);
bool cmp_write_long(cmp_ctx_t *ctx, int64_t d);

bool cmp_read_int(cmp_ctx_t *ctx, int32_t *i);
bool cmp_read_long(cmp_ctx_t *ctx, int64_t *d);

As I see, currently to read a huge binary data, they need to be written to the buffer by reading function. What I would like to have is to get a pointer to memory-mapped binary instead of providing a buffer that will be eventually filled with binary data by reader function.