From 77a334a2af6cff3b055eb978078bba85ac09c9ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Florian=20M=C3=A4rkl?= Date: Wed, 25 Jan 2023 16:42:46 +0100 Subject: [PATCH] Refactor srec reading to not use sscanf (#3321) Parsing with sscanf turned out to cause an extreme slowdown. As only hex values are being read, this can be done with RzUtil-provided functionality. Relevant test cases exist in test/db/io/srec. --- librz/io/p/io_srec.c | 206 ++++++++++++++++++------------------------- librz/util/hex.c | 2 +- test/db/io/srec | 1 - 3 files changed, 85 insertions(+), 124 deletions(-) diff --git a/librz/io/p/io_srec.c b/librz/io/p/io_srec.c index 199c28876e..3d5adce035 100644 --- a/librz/io/p/io_srec.c +++ b/librz/io/p/io_srec.c @@ -134,6 +134,52 @@ static bool __plugin_open(RzIO *io, const char *pathname, bool many) { return (!strncmp(pathname, SREC_PATH_PREFIX, strlen(SREC_PATH_PREFIX))); } +/** + * Parse a value formatted like "%02x" of exactly 2 chars + */ +static bool parse_hex_byte(const char *str, RZ_OUT ut8 *out) { + if (!str[0] || !str[1]) { + return false; + } + *out = 0; + return !rz_hex_to_byte(out, str[0]) && !rz_hex_to_byte(out, str[1]); +} + +/** + * Parse an n-byte big endian hex value of exactly 2*n chars + */ +static bool parse_hex_bytes(const char *str, size_t count, RZ_OUT ut32 *out) { + *out = 0; + while (count--) { + ut8 byte; + if (!parse_hex_byte(str, &byte)) { + return false; + } + *out = *out << 8 | (ut32)byte; + str += 2; + } + return true; +} + +/** + * Parse a prefix formatted like "S%c%02x" of exactly 4 chars + */ +static bool parse_srecord_prefix(const char *str, RZ_OUT char *record_type, RZ_OUT st32 *byte_count) { + if (*str != 'S') { + return false; + } + *record_type = str[1]; + if (!*record_type) { + return false; + } + ut8 bc; + if (!parse_hex_byte(str + 2, &bc)) { + return false; + } + *byte_count = bc; + return true; +} + static bool srecord_parse(RzBuffer *buf, char *str) { if (!str || *str != 'S') { return false; @@ -147,7 +193,7 @@ static bool srecord_parse(RzBuffer *buf, char *str) { char *eol = NULL; ut8 cksum = 0; st32 byte_count = 0; - st32 byte = 0, i = 0, counter = 0; + st32 i = 0, counter = 0; int line = 0; record_data = malloc(UT16_MAX); @@ -157,8 +203,8 @@ static bool srecord_parse(RzBuffer *buf, char *str) { do { line++; - if (sscanf(str, "S%c%02x", &record_type, &byte_count) != 2) { - RZ_LOG_ERROR("srec:parse(): invalid data in motorola srecord file at line %d\n", line); + if (!parse_srecord_prefix(str, &record_type, &byte_count)) { + RZ_LOG_ERROR("srec:parse(): invalid prefix in Motorola S-record file at line %d\n", line); goto fail; } @@ -167,7 +213,7 @@ static bool srecord_parse(RzBuffer *buf, char *str) { switch (record_type) { case '0': // Header with 16-bit address counter = byte_count - 3; - if (sscanf(str + 4, "%04x", &record_addr) != 1) { + if (!parse_hex_bytes(str + 4, 2, &record_addr)) { RZ_LOG_ERROR("srec:parse(): invalid header hexadecimal address 16-bit at line %d\n", line); goto fail; } @@ -178,28 +224,18 @@ static bool srecord_parse(RzBuffer *buf, char *str) { cksum += record_addr >> 8; for (i = 0; i < counter; i++) { - if (sscanf(str + 8 + (i * 2), "%02x", &byte) != 1) { + ut8 byte; + if (!parse_hex_byte(str + 8 + (i * 2), &byte)) { RZ_LOG_ERROR("srec:parse(): invalid hexadecimal value! at line %d\n", line); goto fail; } cksum += byte; } - cksum = ~cksum; - - if (sscanf(str + 2 + (byte_count * 2), "%02x", &byte) != 1) { - RZ_LOG_ERROR("srec:parse(): invalid hexadecimal value! at line %d\n", line); - goto fail; - } else if (cksum != byte) { - RZ_LOG_ERROR("srec:parse(): checksum check failed (got %02x expected %02x) at line %d\n", byte, cksum, line); - goto fail; - } - - str = strchr(str + 1, 'S'); break; case '1': // Data with 16-bit address counter = byte_count - 3; - if (sscanf(str + 4, "%04x", &record_addr) != 1) { + if (!parse_hex_bytes(str + 4, 2, &record_addr)) { RZ_LOG_ERROR("srec:parse(): invalid data hexadecimal address 16-bit at line %d\n", line); goto fail; } @@ -222,7 +258,8 @@ static bool srecord_parse(RzBuffer *buf, char *str) { } for (i = 0; i < byte_count - 3; i++) { - if (sscanf(str + 8 + (i * 2), "%02x", &byte) != 1) { + ut8 byte; + if (!parse_hex_byte(str + 8 + (i * 2), &byte)) { RZ_LOG_ERROR("srec:parse(): invalid hexadecimal value at line %d\n", line); goto fail; } @@ -231,69 +268,34 @@ static bool srecord_parse(RzBuffer *buf, char *str) { } cksum += byte; } - cksum = ~cksum; record_size += counter; record_next += counter; - - if (sscanf(str + 2 + (byte_count * 2), "%02x", &byte) != 1) { - RZ_LOG_ERROR("srec:parse(): invalid hexadecimal value at line %d\n", line); - goto fail; - } else if (cksum != byte) { - RZ_LOG_ERROR("srec:parse(): checksum check failed (got %02x expected %02x) at line %d\n", byte, cksum, line); - goto fail; - } - - str = strchr(str + 1, 'S'); break; case '5': // Count with 16-bit address - if (sscanf(str + 4, "%04x", &record_addr) != 1) { + if (!parse_hex_bytes(str + 4, 2, &record_addr)) { RZ_LOG_ERROR("srec:parse(): invalid count hexadecimal address 16-bit at line %d\n", line); goto fail; } record_addr &= 0xffff; - cksum = byte_count; cksum += record_addr & 0xff; cksum += record_addr >> 8; - cksum = ~cksum; - - if (sscanf(str + 2 + (byte_count * 2), "%02x", &byte) != 1) { - RZ_LOG_ERROR("srec:parse(): invalid hexadecimal value at line %d\n", line); - goto fail; - } else if (cksum != byte) { - RZ_LOG_ERROR("srec:parse(): checksum check failed (got %02x expected %02x) at line %d\n", byte, cksum, line); - goto fail; - } - - str = strchr(str + 1, 'S'); break; case '9': // Terminator with 16-bit address - if (sscanf(str + 4, "%04x", &record_addr) != 1) { + if (!parse_hex_bytes(str + 4, 2, &record_addr)) { RZ_LOG_ERROR("srec:parse(): invalid terminator hexadecimal address 16-bit at line %d\n", line); goto fail; } record_addr &= 0xffff; - cksum = byte_count; cksum += record_addr & 0xff; cksum += record_addr >> 8; - cksum = ~cksum; - - if (sscanf(str + 2 + (byte_count * 2), "%02x", &byte) != 1) { - RZ_LOG_ERROR("srec:parse(): invalid hexadecimal value at line %d\n", line); - goto fail; - } else if (cksum != byte) { - RZ_LOG_ERROR("srec:parse(): checksum check failed (got %02x expected %02x) at line %d\n", byte, cksum, line); - goto fail; - } - - str = strchr(str + 1, 'S'); break; case '2': // Data with 24-bit address counter = byte_count - 4; - if (sscanf(str + 4, "%06x", &record_addr) != 1) { + if (!parse_hex_bytes(str + 4, 3, &record_addr)) { RZ_LOG_ERROR("srec:parse(): invalid data hexadecimal address 24-bit at line %d\n", line); goto fail; } @@ -317,7 +319,8 @@ static bool srecord_parse(RzBuffer *buf, char *str) { } for (i = 0; i < byte_count - 4; i++) { - if (sscanf(str + 10 + (i * 2), "%02x", &byte) != 1) { + ut8 byte; + if (!parse_hex_byte(str + 10 + (i * 2), &byte)) { RZ_LOG_ERROR("srec:parse(): invalid hexadecimal value at line %d\n", line); goto fail; } @@ -326,47 +329,24 @@ static bool srecord_parse(RzBuffer *buf, char *str) { } cksum += byte; } - cksum = ~cksum; record_size += counter; record_next += counter; - - if (sscanf(str + 2 + (byte_count * 2), "%02x", &byte) != 1) { - RZ_LOG_ERROR("srec:parse(): invalid hexadecimal value at line %d\n", line); - goto fail; - } else if (cksum != byte) { - RZ_LOG_ERROR("srec:parse(): checksum check failed (got %02x expected %02x) at line %d\n", byte, cksum, line); - goto fail; - } - - str = strchr(str + 1, 'S'); break; case '6': // Count with 24-bit address - if (sscanf(str + 4, "%06x", &record_addr) != 1) { + if (!parse_hex_bytes(str + 4, 3, &record_addr)) { RZ_LOG_ERROR("srec:parse(): invalid hexadecimal address 24-bit at line %d\n", line); goto fail; } record_addr &= 0xffffff; - cksum = byte_count; cksum += record_addr & 0xff; cksum += (record_addr >> 8) & 0xff; cksum += record_addr >> 16; - cksum = ~cksum; - - if (sscanf(str + 2 + (byte_count * 2), "%02x", &byte) != 1) { - RZ_LOG_ERROR("srec:parse(): invalid hexadecimal value at line %d\n", line); - goto fail; - } else if (cksum != byte) { - RZ_LOG_ERROR("srec:parse(): checksum check failed (got %02x expected %02x) at line %d\n", byte, cksum, line); - goto fail; - } - - str = strchr(str + 1, 'S'); break; case '8': // Terminator with 24-bit address - if (sscanf(str + 4, "%06x", &record_addr) != 1) { + if (!parse_hex_bytes(str + 4, 3, &record_addr)) { RZ_LOG_ERROR("srec:parse(): invalid hexadecimal address 24-bit at line %d\n", line); goto fail; } @@ -381,22 +361,11 @@ static bool srecord_parse(RzBuffer *buf, char *str) { cksum += record_addr & 0xff; cksum += (record_addr >> 8) & 0xff; cksum += record_addr >> 16; - cksum = ~cksum; - - if (sscanf(str + 2 + (byte_count * 2), "%02x", &byte) != 1) { - RZ_LOG_ERROR("srec:parse(): invalid hexadecimal value at line %d\n", line); - goto fail; - } else if (cksum != byte) { - RZ_LOG_ERROR("srec:parse(): checksum check failed (got %02x expected %02x) at line %d\n", byte, cksum, line); - goto fail; - } - - str = strchr(str + 1, 'S'); break; case '3': // Data with 32-bit address counter = byte_count - 5; - if (sscanf(str + 4, "%08x", &record_addr) != 1) { + if (!parse_hex_bytes(str + 4, 4, &record_addr)) { RZ_LOG_ERROR("srec:parse(): invalid hexadecimal address 32-bit at line %d\n", line); goto fail; } @@ -426,7 +395,8 @@ static bool srecord_parse(RzBuffer *buf, char *str) { } for (i = 0; i < byte_count - 5; i++) { - if (sscanf(str + 12 + (i * 2), "%02x", &byte) != 1) { + ut8 byte; + if (!parse_hex_byte(str + 12 + (i * 2), &byte)) { RZ_LOG_ERROR("srec:parse(): invalid hexadecimal value at line %d\n", line); goto fail; } @@ -435,23 +405,12 @@ static bool srecord_parse(RzBuffer *buf, char *str) { } cksum += byte; } - cksum = ~cksum; record_size += counter; record_next += counter; - - if (sscanf(str + 2 + (byte_count * 2), "%02x", &byte) != 1) { - RZ_LOG_ERROR("srec:parse(): invalid hexadecimal value at line %d\n", line); - goto fail; - } else if (cksum != byte) { - RZ_LOG_ERROR("srec:parse(): checksum check failed (got %02x expected %02x) at line %d\n", byte, cksum, line); - goto fail; - } - - str = strchr(str + 1, 'S'); break; case '7': // Terminator with 32-bit address - if (sscanf(str + 4, "%08x", &record_addr) != 1) { + if (!parse_hex_bytes(str + 4, 4, &record_addr)) { RZ_LOG_ERROR("srec:parse(): invalid hexadecimal address 32-bit at line %d\n", line); goto fail; } @@ -467,25 +426,28 @@ static bool srecord_parse(RzBuffer *buf, char *str) { cksum += (record_addr >> 8) & 0xff; cksum += (record_addr >> 16) & 0xff; cksum += record_addr >> 24; - cksum = ~cksum; - - if (sscanf(str + 2 + (byte_count * 2), "%02x", &byte) != 1) { - RZ_LOG_ERROR("srec:parse(): invalid hexadecimal value at line %d\n", line); - goto fail; - } else if (cksum != byte) { - RZ_LOG_ERROR("srec:parse(): checksum check failed (got %02x expected %02x) at line %d\n", byte, cksum, line); - goto fail; - } - - str = strchr(str + 1, 'S'); break; - case '4': - break; + case '4': // reserved + continue; default: - RZ_LOG_ERROR("srec:parse(): invalid motorola srecord type '%c' at line %d\n", record_type, line); + RZ_LOG_ERROR("srec:parse(): invalid Motorola S-record type '%c' at line %d\n", record_type, line); goto fail; } + + // Validate checksum + cksum = ~cksum; + ut8 cksum_expect; + if (!parse_hex_byte(str + 2 + (byte_count * 2), &cksum_expect)) { + RZ_LOG_ERROR("srec:parse(): invalid hexadecimal value! at line %d\n", line); + goto fail; + } + if (cksum != cksum_expect) { + RZ_LOG_ERROR("srec:parse(): checksum test failed (calculated %02x, but expected %02x) at line %d\n", cksum, cksum_expect, line); + goto fail; + } + + str = strchr(str + 1, 'S'); } while (str); if (record_size && record_size < UT16_MAX) { @@ -566,4 +528,4 @@ RZ_API RzLibStruct rizin_plugin = { .data = &rz_io_plugin_srec, .version = RZ_VERSION }; -#endif \ No newline at end of file +#endif diff --git a/librz/util/hex.c b/librz/util/hex.c index 1da7df7e04..4cc0793095 100644 --- a/librz/util/hex.c +++ b/librz/util/hex.c @@ -6,7 +6,7 @@ #include #include -/* int c; ret = hex_to_byte(&c, 'c'); */ +/* int c = 0; ret = hex_to_byte(&c, 'c'); */ RZ_API bool rz_hex_to_byte(ut8 *val, ut8 c) { if (IS_DIGIT(c)) { *val = (ut8)(*val) * 16 + (c - '0'); diff --git a/test/db/io/srec b/test/db/io/srec index 795c9c2ab5..0c04c1cbbb 100644 --- a/test/db/io/srec +++ b/test/db/io/srec @@ -81,7 +81,6 @@ RUN NAME=arm1.bin.srec FILE=srec://bins/srec/arm1.bin.srec -TIMEOUT=1320 CMDS=<