From d812acff246886f8ac8ae33b7afcb376d6e6b10e Mon Sep 17 00:00:00 2001 From: Rangi42 Date: Sat, 2 Mar 2024 23:08:20 -0500 Subject: [PATCH] Check RGBGFX warning/error format strings with `format_` macro --- include/gfx/main.hpp | 12 +++++-- src/gfx/main.cpp | 71 ++++++++++++++++++++++------------------ src/gfx/pal_spec.cpp | 40 +++++++++++----------- src/gfx/process.cpp | 6 ++-- src/gfx/reverse.cpp | 13 ++++---- test/gfx/randtilegen.cpp | 2 +- test/gfx/rgbgfx_test.cpp | 6 ++-- 7 files changed, 81 insertions(+), 69 deletions(-) diff --git a/include/gfx/main.hpp b/include/gfx/main.hpp index 55189fbf..b25cd1d7 100644 --- a/include/gfx/main.hpp +++ b/include/gfx/main.hpp @@ -72,15 +72,21 @@ extern Options options; /* * Prints a warning, and does not change the error count */ -void warning(char const *fmt, ...); +void warning(char const *fmt, ...) format_(printf, 1, 2); /* * Prints an error, and increments the error count */ -void error(char const *fmt, ...); +void error(char const *fmt, ...) format_(printf, 1, 2); +/* + * Prints an error, and increments the error count + * Does not take format arguments so `format_` and `-Wformat-security` won't complain about + * calling `errorMessage(msg)`. + */ +void errorMessage(char const *msg); /* * Prints a fatal error, increments the error count, and gives up */ -[[noreturn]] void fatal(char const *fmt, ...); +[[noreturn]] void fatal(char const *fmt, ...) format_(printf, 1, 2); struct Palette { // An array of 4 GBC-native (RGB555) colors diff --git a/src/gfx/main.cpp b/src/gfx/main.cpp index 643c5904..2072246f 100644 --- a/src/gfx/main.cpp +++ b/src/gfx/main.cpp @@ -72,6 +72,13 @@ void error(char const *fmt, ...) { nbErrors++; } +void errorMessage(char const *msg) { + fprintf(stderr, "error: %s\n", msg); + + if (nbErrors != std::numeric_limits::max()) + nbErrors++; +} + [[noreturn]] void fatal(char const *fmt, ...) { va_list ap; @@ -111,36 +118,36 @@ static char const *optstring = "-Aa:b:Cc:Dd:FfhL:mN:n:Oo:Pp:Qq:r:s:Tt:U:uVvx:Z"; * over short opt matching */ static option const longopts[] = { - {"auto-attr-map", no_argument, nullptr, 'A'}, - {"output-attr-map", no_argument, nullptr, -'A'}, // Deprecated - {"attr-map", required_argument, nullptr, 'a'}, - {"base-tiles", required_argument, nullptr, 'b'}, - {"color-curve", no_argument, nullptr, 'C'}, - {"colors", required_argument, nullptr, 'c'}, - {"depth", required_argument, nullptr, 'd'}, - {"slice", required_argument, nullptr, 'L'}, - {"mirror-tiles", no_argument, nullptr, 'm'}, - {"nb-tiles", required_argument, nullptr, 'N'}, - {"nb-palettes", required_argument, nullptr, 'n'}, - {"group-outputs", no_argument, nullptr, 'O'}, - {"output", required_argument, nullptr, 'o'}, - {"auto-palette", no_argument, nullptr, 'P'}, - {"output-palette", no_argument, nullptr, -'P'}, // Deprecated - {"palette", required_argument, nullptr, 'p'}, - {"auto-palette-map", no_argument, nullptr, 'Q'}, - {"output-palette-map", no_argument, nullptr, -'Q'}, // Deprecated - {"palette-map", required_argument, nullptr, 'q'}, - {"reverse", required_argument, nullptr, 'r'}, - {"auto-tilemap", no_argument, nullptr, 'T'}, - {"output-tilemap", no_argument, nullptr, -'T'}, // Deprecated - {"tilemap", required_argument, nullptr, 't'}, - {"unit-size", required_argument, nullptr, 'U'}, - {"unique-tiles", no_argument, nullptr, 'u'}, - {"version", no_argument, nullptr, 'V'}, - {"verbose", no_argument, nullptr, 'v'}, - {"trim-end", required_argument, nullptr, 'x'}, - {"columns", no_argument, nullptr, 'Z'}, - {nullptr, no_argument, nullptr, 0 } + {"auto-attr-map", no_argument, nullptr, 'A'}, + {"output-attr-map", no_argument, nullptr, -'A'}, // Deprecated + {"attr-map", required_argument, nullptr, 'a'}, + {"base-tiles", required_argument, nullptr, 'b'}, + {"color-curve", no_argument, nullptr, 'C'}, + {"colors", required_argument, nullptr, 'c'}, + {"depth", required_argument, nullptr, 'd'}, + {"slice", required_argument, nullptr, 'L'}, + {"mirror-tiles", no_argument, nullptr, 'm'}, + {"nb-tiles", required_argument, nullptr, 'N'}, + {"nb-palettes", required_argument, nullptr, 'n'}, + {"group-outputs", no_argument, nullptr, 'O'}, + {"output", required_argument, nullptr, 'o'}, + {"auto-palette", no_argument, nullptr, 'P'}, + {"output-palette", no_argument, nullptr, -'P'}, // Deprecated + {"palette", required_argument, nullptr, 'p'}, + {"auto-palette-map", no_argument, nullptr, 'Q'}, + {"output-palette-map", no_argument, nullptr, -'Q'}, // Deprecated + {"palette-map", required_argument, nullptr, 'q'}, + {"reverse", required_argument, nullptr, 'r'}, + {"auto-tilemap", no_argument, nullptr, 'T'}, + {"output-tilemap", no_argument, nullptr, -'T'}, // Deprecated + {"tilemap", required_argument, nullptr, 't'}, + {"unit-size", required_argument, nullptr, 'U'}, + {"unique-tiles", no_argument, nullptr, 'u'}, + {"version", no_argument, nullptr, 'V'}, + {"verbose", no_argument, nullptr, 'v'}, + {"trim-end", required_argument, nullptr, 'x'}, + {"columns", no_argument, nullptr, 'Z'}, + {nullptr, no_argument, nullptr, 0 } }; static void printUsage() { @@ -334,7 +341,7 @@ static std::vector readAtFile(std::string const &path, std::vector * Returns `nullptr` if the vector was fully parsed, or a pointer (which is part of the arg vector) * to an "at-file" path if one is encountered. */ -static char *parseArgv(int argc, char **argv) { +static char *parseArgv(int argc, char *argv[]) { for (int ch; (ch = musl_getopt_long_only(argc, argv, optstring, longopts, nullptr)) != -1;) { char *arg = musl_optarg; // Make a copy for scanning uint16_t number; @@ -406,7 +413,7 @@ static char *parseArgv(int argc, char **argv) { if (*arg != '\0') { error("Bit depth (-b) argument must be a valid number, not \"%s\"", musl_optarg); } else if (options.bitDepth != 1 && options.bitDepth != 2) { - error("Bit depth must be 1 or 2, not %" PRIu8); + error("Bit depth must be 1 or 2, not %" PRIu8, options.bitDepth); options.bitDepth = 2; } break; diff --git a/src/gfx/pal_spec.cpp b/src/gfx/pal_spec.cpp index 3442a958..66228d12 100644 --- a/src/gfx/pal_spec.cpp +++ b/src/gfx/pal_spec.cpp @@ -59,13 +59,12 @@ void parseInlinePalSpec(char const * const rawArg) { std::string_view arg(rawArg); using size_type = decltype(arg)::size_type; - auto parseError = [&rawArg, &arg](size_type ofs, size_type len, char const *fmt, - auto &&...args) { + auto parseError = [&rawArg, &arg](size_type ofs, size_type len, char const *msg) { (void)arg; // With NDEBUG, `arg` is otherwise not used assert(ofs <= arg.length()); assert(len <= arg.length()); - error(fmt, args...); + errorMessage(msg); fprintf(stderr, "In inline palette spec: %s\n" " ", @@ -237,31 +236,31 @@ static std::optional parseColor(std::string const &str, std::string::size_ uint16_t i) { std::optional r = parseDec(str, n); if (!r) { - error("Failed to parse color #%" PRIu16 " (\"%s\"): invalid red component", i + 1, + error("Failed to parse color #%d (\"%s\"): invalid red component", i + 1, str.c_str()); return std::nullopt; } skipWhitespace(str, n); if (n == str.length()) { - error("Failed to parse color #%" PRIu16 " (\"%s\"): missing green component", i + 1, + error("Failed to parse color #%d (\"%s\"): missing green component", i + 1, str.c_str()); return std::nullopt; } std::optional g = parseDec(str, n); if (!g) { - error("Failed to parse color #%" PRIu16 " (\"%s\"): invalid green component", i + 1, + error("Failed to parse color #%d (\"%s\"): invalid green component", i + 1, str.c_str()); return std::nullopt; } skipWhitespace(str, n); if (n == str.length()) { - error("Failed to parse color #%" PRIu16 " (\"%s\"): missing blue component", i + 1, + error("Failed to parse color #%d (\"%s\"): missing blue component", i + 1, str.c_str()); return std::nullopt; } std::optional b = parseDec(str, n); if (!b) { - error("Failed to parse color #%" PRIu16 " (\"%s\"): invalid blue component", i + 1, + error("Failed to parse color #%d (\"%s\"): invalid blue component", i + 1, str.c_str()); return std::nullopt; } @@ -295,11 +294,11 @@ static void parsePSPFile(std::filebuf &file) { return; } - if (*nbColors > options.nbColorsPerPal * options.nbPalettes) { + if (uint16_t nbPalColors = options.nbColorsPerPal * options.nbPalettes; *nbColors > nbPalColors) { warning("PSP file contains %" PRIu16 " colors, but there can only be %" PRIu16 "; ignoring extra", - *nbColors, options.nbColorsPerPal * options.nbPalettes); - nbColors = options.nbColorsPerPal * options.nbPalettes; + *nbColors, nbPalColors); + nbColors = nbPalColors; } options.palSpec.clear(); @@ -314,8 +313,7 @@ static void parsePSPFile(std::filebuf &file) { return; } if (n != line.length()) { - error("Failed to parse color #%" PRIu16 - " (\"%s\"): trailing characters after blue component", + error("Failed to parse color #%d (\"%s\"): trailing characters after blue component", i + 1, line.c_str()); return; } @@ -388,7 +386,7 @@ static void parseHEXFile(std::filebuf &file) { if (line.length() != 6 || line.find_first_not_of("0123456789ABCDEFabcdef"sv) != std::string::npos) { - error("Failed to parse color #%" PRIu16 " (\"%s\"): invalid \"rrggbb\" line", + error("Failed to parse color #%d (\"%s\"): invalid \"rrggbb\" line", nbColors + 1, line.c_str()); return; } @@ -431,11 +429,11 @@ static void parseACTFile(std::filebuf &file) { return; } - if (nbColors > options.nbColorsPerPal * options.nbPalettes) { + if (uint16_t nbPalColors = options.nbColorsPerPal * options.nbPalettes; nbColors > nbPalColors) { warning("ACT file contains %" PRIu16 " colors, but there can only be %" PRIu16 "; ignoring extra", - nbColors, options.nbColorsPerPal * options.nbPalettes); - nbColors = options.nbColorsPerPal * options.nbPalettes; + nbColors, nbPalColors); + nbColors = nbPalColors; } options.palSpec.clear(); @@ -481,18 +479,18 @@ static void parseACOFile(std::filebuf &file) { } uint16_t nbColors = readBE(buf); - if (nbColors > options.nbColorsPerPal * options.nbPalettes) { + if (uint16_t nbPalColors = options.nbColorsPerPal * options.nbPalettes; nbColors > nbPalColors) { warning("ACO file contains %" PRIu16 " colors, but there can only be %" PRIu16 "; ignoring extra", - nbColors, options.nbColorsPerPal * options.nbPalettes); - nbColors = options.nbColorsPerPal * options.nbPalettes; + nbColors, nbPalColors); + nbColors = nbPalColors; } options.palSpec.clear(); for (uint16_t i = 0; i < nbColors; ++i) { if (file.sgetn(buf, 10) != 10) { - error("Failed to read color #%" PRIu16 " from palette file", i + 1); + error("Failed to read color #%d from palette file", i + 1); return; } diff --git a/src/gfx/process.cpp b/src/gfx/process.cpp index 62784aa3..2fe58542 100644 --- a/src/gfx/process.cpp +++ b/src/gfx/process.cpp @@ -106,9 +106,9 @@ class Png { if (nbBytesRead != expectedLen) { fatal("Error reading input image (\"%s\"): file too short (expected at least %zd more " - "bytes after reading %lld)", + "bytes after reading %zu)", self->c_str(), length - nbBytesRead, - self->file->pubseekoff(0, std::ios_base::cur)); + (size_t)self->file->pubseekoff(0, std::ios_base::cur)); } } @@ -1060,7 +1060,7 @@ void process() { } if (nbColorsInTile > options.maxOpaqueColors()) { - fatal("Tile at (%" PRIu32 ", %" PRIu32 ") has %zu opaque colors, more than %" PRIu8 "!", + fatal("Tile at (%" PRIu32 ", %" PRIu32 ") has %" PRIu8 " opaque colors, more than %" PRIu8 "!", tile.x, tile.y, nbColorsInTile, options.maxOpaqueColors()); } diff --git a/src/gfx/reverse.cpp b/src/gfx/reverse.cpp index e0fcec9d..2e1b28e4 100644 --- a/src/gfx/reverse.cpp +++ b/src/gfx/reverse.cpp @@ -95,7 +95,7 @@ void reverse() { } if (options.inputSlice.width != 0 && options.inputSlice.width != options.reversedWidth * 8) { warning("Specified input slice width (%" PRIu16 - ") doesn't match provided reversing width (%" PRIu8 " * 8)", + ") doesn't match provided reversing width (%" PRIu16 " * 8)", options.inputSlice.width, options.reversedWidth); } @@ -121,8 +121,8 @@ void reverse() { fatal("Cannot generate empty image"); } if (nbTileInstances > options.maxNbTiles[0] + options.maxNbTiles[1]) { - warning("Read %zu tiles, more than the limit of %zu + %zu", nbTileInstances, - options.maxNbTiles[0], options.maxNbTiles[1]); + warning("Read %zu tiles, more than the limit of %" PRIu16 " + %" PRIu16, + nbTileInstances, options.maxNbTiles[0], options.maxNbTiles[1]); } size_t width = options.reversedWidth, height; // In tiles @@ -167,8 +167,8 @@ void reverse() { } while (nbRead != 0); if (palettes.size() > options.nbPalettes) { - warning("Read %zu palettes, more than the specified limit of %zu", palettes.size(), - options.nbPalettes); + warning("Read %zu palettes, more than the specified limit of %" PRIu8, + palettes.size(), options.nbPalettes); } if (options.palSpecType == Options::EXPLICIT && palettes != options.palSpec) { @@ -195,7 +195,8 @@ void reverse() { bool bad = false; for (auto attr : *attrmap) { if ((attr & 0b111) > palettes.size()) { - error("Referencing palette %u, but there are only %zu!"); + error("Referencing palette %u, but there are only %zu!", + attr & 0b111, palettes.size()); bad = true; } if (attr & 0x08 && !tilemap) { diff --git a/test/gfx/randtilegen.cpp b/test/gfx/randtilegen.cpp index a4c8e7a4..c6f5a836 100644 --- a/test/gfx/randtilegen.cpp +++ b/test/gfx/randtilegen.cpp @@ -260,7 +260,7 @@ static void generate_random_image(char const *filename) { write_image(filename, palettes, tileData, attributes, width, height); } -int main(int argc, char **argv) { +int main(int argc, char *argv[]) { if (argc < 3 || argc > 4) { fprintf(stderr, "usage: %s []\n", argv[0]); return 2; diff --git a/test/gfx/rgbgfx_test.cpp b/test/gfx/rgbgfx_test.cpp index b8c0650a..9f8091a0 100644 --- a/test/gfx/rgbgfx_test.cpp +++ b/test/gfx/rgbgfx_test.cpp @@ -112,9 +112,9 @@ class Png { if (nbBytesRead != expectedLen) { fatal("Error reading input image (\"%s\"): file too short (expected at least %zd more " - "bytes after reading %lld)", + "bytes after reading %zu)", self->path.c_str(), length - nbBytesRead, - self->file.pubseekoff(0, std::ios_base::cur)); + (size_t)self->file.pubseekoff(0, std::ios_base::cur)); } } @@ -369,7 +369,7 @@ static char *execProg(char const *name, char * const *argv) { return nullptr; } -int main(int argc, char **argv) { +int main(int argc, char *argv[]) { if (argc < 2) { fprintf(stderr, "usage: %s [rgbgfx flags]\n", argv[0]); exit(0);