On 19 December, 2014 - Jef Driesen wrote:
On 2014-12-17 23:11, Anton Lundin wrote:
+// This is a variant of fletcher16 with 16 bit sum instead of 8bit +// and modulo 256 instead of 255 +static unsigned int hw_ostc3_firmware_checksum(hw_ostc3_firmware_t *firmware)
The comment is wrong. It's modulo 65536 (2^16) vs 65535 (2^16-1) due to the 16bit sum.
Yea, you're correct. I just wanted to document that its not fletcher16 or fletcher32, but some sort of in-between.
+static dc_status_t +hw_ostc3_firmware_readline(FILE *fp, unsigned int addr, unsigned char data[], unsigned int size) +{ + unsigned char ascii[40]; + // 1 byte :, 6 bytes addr, X*2 bytes hex -> X bytes data. + const unsigned line_size = size * 2 + 1 + 6 + 1; + unsigned char faddr_byte[3]; + unsigned int faddr = 0; + if (line_size > sizeof(ascii)) + return DC_STATUS_INVALIDARGS; + if (fread (ascii, sizeof(char), line_size, fp) != line_size) + return DC_STATUS_IO; + if (ascii[0] != ':') + return DC_STATUS_DATAFORMAT; + // Is it a CRLF file? + // Read away that trailing LF + if (ascii[line_size] == '\r') + if (fread (ascii + line_size, sizeof(char), 1, fp) != 1) + return DC_STATUS_DATAFORMAT; + if (array_convert_hex2bin(ascii + 1, 6, faddr_byte, sizeof(faddr_byte)) != 0) { + return DC_STATUS_DATAFORMAT; + } + faddr = array_uint24_be (faddr_byte); + if (faddr != addr) + return DC_STATUS_DATAFORMAT; + if (array_convert_hex2bin (ascii + 1 + 6, size*2, data, size) != 0) + return DC_STATUS_DATAFORMAT; + + return DC_STATUS_SUCCESS; +}
There are two bugs in this code. You read line_size bytes, and then try to access ascii[line_size], which is left uninitialized. The ascii array is also not large enough to store CRLF, resulting in a buffer overflow.
There are two -1 that was missing there to make the code do what i ment for it to do. I never ment to store CRLF, only LF, so if there is a CRLF ending, throw away the CR.
I have attached a different implementation, which is based on the existing ihex code. It can handle arbitrary line endings, and not just CRLF or LF. So if you don't mind, I prefer to use this version.
I actually think its uglier, and thats why i didn't copy that pattern. But if thats your preferred way to read data, you're the maintainer. And... a bit more thinking. The firmware files are distributed inside zip-files. If you do anything to them, i would say its a feature to reject that file. The odds of flipping some bits that decrypt into something that matches the checksum are pretty slim, but i wouldn't trust a firmware file that had a blank line in it... //Anton -- Anton Lundin +46702-161604