Update to new file_io messages #452
Conversation
|
Test PASSed. |
| @@ -81,16 +81,16 @@ static void read_cb(u16 sender_id, u8 len, u8 msg[], void* context) | |||
| len += cfs_read(f, buf + len, readlen); | |||
| cfs_close(f); | |||
|
|
|||
| sbp_send_msg(SBP_MSG_FILEIO_READ, len, buf); | |||
| sbp_send_msg(SBP_MSG_FILEIO_READ_RESPONSE, len, buf); | |||
There was a problem hiding this comment.
above here (line 77), we need a check that len + readlen < 256, otherwise the you could easily overrun buffer[256]. I don't think we can trust the sender of the message took into account that readlen needs to be correct.
I had the check:
if (len + readlen > 256) {
log_error("Error in read_cb, requested length too long. Length was %d, filename is %s", readlen, &msg[5]);
}
When I was working on it
There was a problem hiding this comment.
len is an 8-bit value, so you can't overrun the buffer - it'll just wrap. If you try and in a check for this, the compiler is warning:
sbp_fileio.c:86:3: error: comparison is always false due to limited range of data type
|
Something else to consider is whether we also want to do sender_id checks to ensure these messages are not coming from other piksi's: if (sender_id != 0x42)
return; |
|
If we are changing the IDs so there's no ambiguity as to whether a particular message is a Request or a Response, is the sender_id check necessary? |
|
Shouldn't be - just extra protection |
|
Sounds good - I'm all about it. |
|
Would Piksis ever want to program each other or read each other's files? Probably not but worth considering... |
|
Dennis suggested a general purpose wrapper message that could contain another message within itself. The wrapper message could also have flags for what was to be done with the wrapped message (forwarded, consumed, etc). |
|
Not sure we want Piksi's playing with each other that way. We don't have quite the right protection in place,... |
|
The nice thing about the wrapper message is that the complexity of forwarded messages goes away in the protocol. |
|
|
|
I think the Piksi's should cough cough abstain from sending each other any such file messages, wrapped in anything, much less brown paper packet frames (tied up in streams). Would you want to unwrap forwarded messages from strange units? |
|
I don't think we have any demonstrated need for encapsulation or piksi-to-piksi file_io, flash, or settings operations. If there's a well-defined specification and application for encapsulation, we could approach the idea then; otherwise, seems premature. For going out in 1.0, seems like a good idea to lock these things down a little and remove inadvertent updates. |
|
Added the |
|
Test PASSed. |
| @@ -81,25 +83,27 @@ static void read_cb(u16 sender_id, u8 len, u8 msg[], void* context) | |||
| len += cfs_read(f, buf + len, readlen); | |||
There was a problem hiding this comment.
Let's say the original message is 16 bits long as sent (len=16). Lets say the readlen field of the message is its max value (256). the cfs_read operation just overran buf.
There was a problem hiding this comment.
Ah, the u8 len + the u8 readlen in the call to cfs_read could overwrite buf 256 bytes. Added MIN(readlen, 255-len) to determine how much to read into the buffer. Thanks!
|
Test PASSed. |
| (void)context; | ||
|
|
||
| if (sender_id != 0x42) |
There was a problem hiding this comment.
I may have missed this earlier, but sender ID 42 is used by those host computer writing to the flash? This would useful to have defined in a constant (and comment) somewhere in the header.
|
@henryhallam @gsmcmullin minor changes here around the file io messaging paths - could use a brief review. |
|
Rebased. |
|
Test PASSed. |
|
Test PASSed. |
|
Bringing this in. |
Update sbp and move to new file_io messages. See swift-nav/libsbp#183.
/cc @fnoble @denniszollo @mookerji