dtls_debug.h: explicitly cast macro parameter to size_t.

[boaks]

No description provided.

[OlegHahm]

As far as I understand the test results, the Windows toolchain does not support %zu. Should we use %lu instead then? May waste some bytes on some platforms but that shouldn't be an issue for debug output.

[boaks]

No need. PR #218 or #216 will fix it.

(I will ask Olaf an Jon again, if we could decide the way to go.)

[obgm]

I would refrain from casting to size_t in any macro because we do not know what the input parameters are. Keep in mind that the size of size_t depends on the target platform, and that people sometimes use int for counting things. A type cast therefore should occur where the parameter is passed to this macro.
Anyway, PR #218 is merged know, so the particular issue of not having %zu should be fixed.

[boaks]

I'm not sure, if I get it.

The macros are all dumping an array of bytes with a certain length.
The default calls:

void dtls_dsrv_hexdump_log(log_t level, const char *name, const unsigned char *buf, size_t length, int extend); which casts the length implicit to size_t).

so, if we want similar behavior, we need to add that cast to the specific versions for zephyr and riot.

Or did I miss something?

[boaks]

I've checked this again, and I still don't see the point to "refrain from that cast to size_t".

What this PR is fixing is the assumption in the LOG_DBG/LOG_DEBUG, where the length is passed to "%zu". I don't see, why that should harm. On the other side, if this is required to be done at the call, then this will introduce an error source, if not done. That would also cause to fix several locations within tinydtls itself, where we then need to ensure, that "%zu" gets passed the proper type.

[mrdeep1]

I'm inclined to agree with obgm, based on the law of unintended consequences.

As I read the code, dtls_dsrv_hexdump_log() is always there (apart from CONTIKI), so why not have the stub functions in dtls_debug.c handling Zephyr, RIOT etc. which then call the dedicated functions as the #define currently does.

[boaks]

Using macros in Zephyr (and RIOT?) allows to remove the logging code of some logging levels completely, so that they don't consume storage.

based on the law of unintended consequences.

The code is

#define dtls_debug_hexdump(name, buf, length) { if (LOG_DEBUG <= LOG_LEVEL) { LOG_DEBUG("-= %s (%zu bytes) =-\n", name, (size_t) length); od_hex_dump(buf, length, 0); }}

so I see the "unintended consequences" in not casting length.

[obgm]

so I see the "unintended consequences" in not casting length.

The problem is that dtls_debug_hexdump is part of the public API. And here is what will happen in user applications:

int length = some_custom_function();
const char *data = some_other_custom_function;

dtls_debug_hexdump("foo", data, length);

This happens all the time. Really.

[boaks]

This happens all the time. Really.

Sorry, I may be blind and/or stupid.

What makes the difference of

int length = some_custom_function();
const char *data = some_other_custom_function;
LOG_DEBUG("-= %s (%zu bytes) =-\n", name, (size_t) length); 

int length = some_custom_function();
const char *data = some_other_custom_function;
LOG_DEBUG("-= %s (%zu bytes) =-\n", name, length); 

In the second case, we may get a warning, or the printed result is somehow random.
The first case works in my experience.

[boaks]

dtls_debug_hexdump("foo", data, length);

As I wrote above, this is expanded in a call to

void dtls_dsrv_hexdump_log(log_t level, const char *name, const unsigned char *buf, size_t length, int extend)

which implicitly casts the length to size_t.
Why is then the added cast so bad?
And again, if this is not added, we need to fix several locations with tinydtls itself, because there we pass in parameter, which are not "size_t".

[mrdeep1]

An unintended consequence is when an unexpected negative int value gets expanded into a huge number when cast to a size_t which then causes the app to blow up when doing the following hex dump. The safer way is make sure all the callers have a size_t length parameter.

[boaks]

Therefore the parameter length is only casted for the "LOG_DEBUG/LOG_DBG - %zu" and not for the dump call itself (neither "LOG_HEXDUMP_DBG" nor "od_hex_dump").

In order to test the "protection" of not casting, I added

  dtls_debug_hexdump("test", data, -1);

to dtls.c/dtls_handle_message and run a build on unix/posix (ubuntu 22.04).
No warnings at compile time. And

DEBG test: (18446744073709551615 bytes):

with crash at runtime.

if -1 is dangerous, it's hard to see the protection to not cast the parameter which is passed to "%zu".

[mrdeep1]

with crash at runtime.
if -1 is dangerous, it's hard to see the protection to not cast the parameter which is passed to "%zu".

I'm guessing the crash is in the hexdump logic, so the invokers of dtls_debug_hexdump() must make sure that there is no possibility of a large length (or negative value) getting passed in, or the dtls_debug_hexdump() does additional sanity checking - preferable.

[boaks]

the crash is in the hexdump logic

Sure.

must make sure that there is no possibility of a large length (or negative value) getting passed in,

Exactly. And the same prerequisite about the value of length applies for the macro with the cast to size_t.
With the current implementation, that added cast doesn't add an error source, that error source is already there.
But it fixes the error for "LOG_DEBUG/LOG_DBG - %zu".

If it's then considered, that the hexdump should be limited, e.g. to 1k, then this is an additional issue and with that we may adapt the macro again.

[obgm]

which implicitly casts the length to size_t.

In my example, length is of type int. The type of size_t depends on the target platform. On linux, this is typically unsigned long (= 8 bytes on 64-bit architectures). AFAIK, on windows, it is unsigned long long. The issue I see is with this parameter being passed to the varargs of printf. I have not checked whether or not passing a shorter type for %zu will cause issues but I prefer to be conservative and pass the correct data type.

we need to fix several locations with tinydtls itself, because there we pass in parameter, which are not "size_t".

Yes but this is not a problem, we just need to do that. But for every cast we have to answer this question: "Is it correct to cast this type to size_t?" If not, we need to change the code because otherwise, it will likely break something in the future. This is why I have a very strong opinion on doing type casts at the origin of the "wrong types".

[boaks]

Which type does "%zu" support? Isn't it "size_t"?
At least according C99 it is.

[obgm]

Correct: size_t.

[boaks]

Then this discussion seems to be based on what ever, but not the code change contained in this PR.
As I demonstrated above, cast or not cast will not prevent the assumed error. Nor does it help with warnings (at least I didn't get warnings).
But the cast will fix the LOG_DEBUG("-= %s (%zu bytes) =-\n", name, length); though it ensures, that "length" is cast to the expected type "size_t".

[mrdeep1]

Ok - unintended consequence.

 int a = -1;
 int b = 5;
 char buf[] = "abcdef";

 dtls_debug_hexdump("test", buf, a+b);

which then translates (using your fix)

LOG_DEBUG("-= %s (%zu bytes) =-\n", buf, (size_t)a + b);

and so LOG_DEBUG is passed a large number rather than 4 (it is possible the resultant sum may overflow and wrap).

Another unintended consequence

 int a = 6
 int * b = &a;
 char buf[] = "abcdef";

 dtls_debug_hexdump("test", buf, *b);

which then translates (using your fix)

LOG_DEBUG("-= %s (%zu bytes) =-\n", buf, (size_t)*b);

No idea here as to what is typed to size_t.

Passed parameters should be wrapped with ( ) as in

#define dtls_debug_hexdump(name, buf, length) { LOG_DBG("%s (%zu bytes):", (name), (size_t)(length)); LOG_HEXDUMP_DBG((buf), (length), (name)); }

I agree that typecasting (if done properly) will get rid of your warnings generated by some compilers.

[mrdeep1]

name should be wrapped as (name) in 2 places per line, as well as buf and length wrapped in LOG_HEXDUMP()

#define dtls_debug_hexdump(name, buf, length) { LOG_DBG("%s (%zu bytes):", (name), (size_t)(length)); LOG_HEXDUMP_DBG((buf), (length), (name)); }
#define dtls_debug_dump(name, buf, length) { LOG_DBG("%s (%zu bytes):", (name), (size_t)(length)); LOG_HEXDUMP_DBG((buf), (length), (name)); }

[boaks]

If we want to start with applying general rules about macros, that will change much more ;-).

[mrdeep1]

Agreed. However, every time we have to change a (#define) line of code, lets correctly fix that line and only correct the other lines that need doing when they need to be updated, so minimizing this particular fix.

[boaks]

I disagree.
The tiny scope if this PR is based on an user request (see #212).
Extending the scope to "general/common rules", which are neither agreed nor discussed before is usually the way the generate quite a lot frustration, because that makes it impossible to improve the project in small steps.

If general rules about the design of macros should be applied to this project, I consider a separate issue for discussion will help much more than to "piggyback" this to tiny PRs.

[mrdeep1]

OK - what works best for you.

[mrdeep1]

Same as previous comment - things need to be wrapped in #define.

[obgm]

Apart from my general concerns documented in #237 I wonder why this cast is necessary here, anyway: od_hex_dump() also requires a size_t as data_len argument. Therefore, there is no point in passing anything else than size_t.

[boaks]

anyway: od_hex_dump() also requires a size_t as data_len argument. Therefore, there is no point in passing anything else than size_t.

You can pass what ever C can convert into a "size_t". Even a uint8_t will work.

[boaks]

@obgm
@mrdeep1
@OlegHahm

This PR contains 1 change copied to 4 locations. It is pending now for 3 months.
I'm very deeply frustrated about the process here.
Please come to a conclusion until end of next week.
It's no problem for me to just close it without merging.
But for sure, I don't want to spend more time in it.

[mrdeep1]

I'm happy for this to be merged as is.

[obgm]

I'm happy for this to be merged as is.

So, then just merge it. I still do not want it but since it is just for debug purposes and most IoT applications usually will not have to deal with numbers that exceed the number space of size_t, I will most likely get over it since many smart people do not seem to have a problem with this PR.

[boaks]

I'm sure, if that causes more trouble than it solves, we will simply get new issues.