Fix problem exactly filling a buffer when encoding. #49
Conversation
The size check suffered from an off by one error. This also avoids problems with overflow.
| @@ -35,7 +35,7 @@ typedef struct _write_state | |||
| ssize_t size; | |||
| } cn_write_state; | |||
|
|
|||
| #define ensure_writable(sz) if ((ws->offset<0) || (ws->offset + (sz) >= ws->size)) { \ | |||
| #define ensure_writable(sz) if ((ws->offset<0) || (ws->size - ws->offset < (sz))) { \ | |||
There was a problem hiding this comment.
This is definitely a good change...
src/cn-encoder.c
Outdated
| @@ -302,6 +302,7 @@ ssize_t cn_cbor_encoder_write(uint8_t *buf, | |||
| const cn_cbor *cb) | |||
| { | |||
| cn_write_state ws = { buf, buf_offset, buf_size }; | |||
| if (ws.size < 0) { return -1; } | |||
There was a problem hiding this comment.
In normal use I would expect it would not happen, but if buf_size has the high bit set and buf_offset is non-zero the check in ensure_writable could underflow. Probably an overabundance of caution, but issue #12 made me think about what could happen in an attack situation.
There was a problem hiding this comment.
Would you like to see this removed? Or maybe change ws.size from ssize_t to size_t? I'd like to see the off by one error fixed. It seems there were attempts to do so since 2015 that never quite made it.
|
I generally prefer to make all these calculations unsigned so I don't run into implementation-defined issues. Unfortunately, -1 means something special for ws.offset, which probably is the reason it was defined as ssize_t. Once you mix signed and unsigned, interesting things happen in C, so if we want to change ws.size to size_t, we might have to change ws.offset, too. In any case, a ws.size of more than half of the address space doesn't sound right, so this seems to be more of an "assert" kind of check. |
|
Also look at #25 which does the same thing |
|
Is anything else needed to get this merged? |
The size check suffered from an off by one error.
This also avoids problems with overflow.