coap_pdu_t inconsistencies regarding data size

[PawelWMS]

The coap_pdu_t struct currently has a size_t "max_size" field and an unsigned short "length" field, thus making it possible to use coap_add_data() and overflow "length". Also, coap_add_data() is using a unsigned int "len" argument to pass the amount of the new data.

It would be good to decide on one specific type and stick to it throughout the code.

[obgm]

I agree that changing the length field to size_t would be a good idea (even if no overflow is possible because a CoAP message cannot exceed 64 KiB and the datatype short is at least 16 bits wide).

Having unsigned int for the len argument of coap_add_data() is perfectly valid in my opinion (being a matter of taste, no real technical reason for or against).

[PawelWMS]

I see. I've suggested size_t, because it's the commonly used type to depict sizes/lengths/numbers of elements, etc. I've suggested to use it everywhere for two main reasons:

1. To avoid any compiler warnings about possible loss of data with the size_t->unsigned int cast on 64-bit builds. In most cases size_t will be a 64-bit unsigned integer then and unsigned int only a 32-bit one.
2. For plain consistency/code cleanness.

Another idea might be using unsigned short only, considering the size limit you've mentioned, but I'm not sure if it's worth the types-juggling in the user-facing APIs.

Also, since the CoAP message cannot be larger than 64KB, it might also be a good idea to add that check inside the function's code. Unless "max_size" is supposed to handle that already, but I couldn't find the code, which would set it to this maximum and it seems you can use coap_pdu_clear() to set it to any arbitrary value.

Please, let me know your thoughts.

[obgm]

Note that the 64 KiB limitation only holds for CoAP over UDP. With TCP, a single message might be as large as 4295033115 B (1 + 4 + 1 + 8 + 2^32 + 65805), c.f. https://tools.ietf.org/html/draft-ietf-core-coap-tcp-tls-05#section-2. This rules out unsigned short and also unsigned int on most platforms as you have correctly stated. size_t hence sounds reasonable.

[PawelWMS]

Great to hear it. In that case I'll try to prepare my suggestion of changes and do a pull request soon.

[obgm]

Thanks. Do you intend to do more complex changes than, e.g., the following?

diff --git a/include/coap/pdu.h b/include/coap/pdu.h
index 7ed482d..2ae6cab 100644
--- a/include/coap/pdu.h
+++ b/include/coap/pdu.h
@@ -231,7 +231,7 @@ typedef struct {
                              *   depending on the memory management
                              *   implementation. */
   unsigned short max_delta; /**< highest option number */
-  unsigned short length;    /**< PDU length (including header, options, data) */
+  size_t length;            /**< PDU length (including header, options, data) */
   unsigned char *data;      /**< payload */
 
 #ifdef WITH_LWIP
@@ -373,7 +373,7 @@ unsigned char *coap_add_option_later(coap_pdu_t *pdu,
  * only once per PDU, otherwise the result is undefined.
  */
 int coap_add_data(coap_pdu_t *pdu,
-                  unsigned int len,
+                  size_t len,
                   const unsigned char *data);
 
 /**
diff --git a/src/pdu.c b/src/pdu.c
index ff73d6a..b4ed6a9 100644
--- a/src/pdu.c
+++ b/src/pdu.c
@@ -231,7 +231,7 @@ coap_add_option_later(coap_pdu_t *pdu, unsigned short type, unsigned int len) {
 }
 
 int
-coap_add_data(coap_pdu_t *pdu, unsigned int len, const unsigned char *data) {
+coap_add_data(coap_pdu_t *pdu, size_t len, const unsigned char *data) {
   assert(pdu);
   assert(pdu->data == NULL);
 

[PawelWMS]

Well that and everything this change will touch. This includes coap_pdu_clear2() for instance.

I was also thinking about changing other arguments/members/variables referring to size, which would include changes in:

• coap_option.length;
• coap_new_pdu2() "size" argument;
• coap_get_tcp_header_type_from_size() "size" argument;
• coap_get_tcp_header_type_from_initbyte() "length" argument;
• all the APIs retrieving length like coap_get_length() or coap_get_tcp_header_length();
• coap_add_option()/coap_add_option2()/coap_add_option_later() "len" argument;

What is your opinion about these changes?

[obgm]

I would prefer to first touch only things that already exist in the develop branch. Hence this would be coap_option.length and coap_add_option() and possibly coap_add_option_later().

[PawelWMS]

Sounds good, I'm ok with that.

[obgm]

Great. Are you going to provide a PR?

[PawelWMS]

Yes. Would it be ok to do a PR first to dthaler's branch? It appears that's the one our project currently relies on and it seems that it's on its way to get merged to this repo at one point.

[obgm]

Okay, please go ahead.

[PawelWMS]

Thank you. In that case would also be ok to include the changes to other APIs (present in that fork) I've mentioned earlier?

[PawelWMS]

Hey, I've tried to check all inconsistencies and it seems this would require deprecating many APIs and creating new ones with changed arguments in addition to very thorough source files changes. For now I've implemented this change where I mainly fix /W4 warnings but also make sure that all casts are successful and don't lead to data loss.

[PawelWMS]

@obgm: Please take a look at this change - it would be great to have these changes in the main repo. We can include them only in dthaler's fork, but that way these changes are only limited to that one repo and make future merging back with your original harder.

[obgm]

Closing this for now as PR #113 took care of the inconsistencies regarding the size type.