Comments (15)
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).
from libcoap.
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:
- 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.
- 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.
from libcoap.
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.
from libcoap.
Great to hear it. In that case I'll try to prepare my suggestion of changes and do a pull request soon.
from libcoap.
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);
from libcoap.
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?
from libcoap.
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().
from libcoap.
Sounds good, I'm ok with that.
from libcoap.
Great. Are you going to provide a PR?
from libcoap.
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.
from libcoap.
Okay, please go ahead.
from libcoap.
Thank you. In that case would also be ok to include the changes to other APIs (present in that fork) I've mentioned earlier?
from libcoap.
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.
from libcoap.
@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.
from libcoap.
Closing this for now as PR #113 took care of the inconsistencies regarding the size type.
from libcoap.
Related Issues (20)
- Failing to set mtu when WOLFSSL_DTLS_MTU is enabled HOT 1
- Assertion !lock->in_callback failed in coap_lock_lock_func() during application shutdown HOT 10
- Build with wolfSSL not loading certificates on server side HOT 3
- Memory leak in LwIP function `coap_udp_recvs` HOT 1
- PDU use after free when sending an OSCORE message with OSCORE disabled at build time HOT 5
- coap-client binary PSK HOT 17
- [Question] Proxy-scheme and proxy-uri example with coap-client and coap-server applications HOT 6
- Make coap_print_wellknown customizable HOT 6
- Wrong
- Access Token option addition to libcoap HOT 1
- Clarification on cancelling a block-wise request in the middle of transaction at client's end HOT 4
- Clarify required buffer size for coap_uri_into_options() and coap_split_uri() HOT 10
- coap_io_process does not return after ppp connection is closed HOT 10
- Name Service Switch interface integration of Coap-RD HOT 1
- Extra quotation mark for unix sockets in coap_print_addr HOT 2
- Specific api for setting the DTLS version HOT 5
- Unexpected token after long running block transfer HOT 9
- coap_send IPv6 HOT 1
- DTLS Connection id (CID) HOT 1
- coap-client gets stuck with large .well-known/core response HOT 3
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from libcoap.