From c4b6926a8721c9791610c73d0ac25902152d37e4 Mon Sep 17 00:00:00 2001 From: waterwheels Date: Sun, 7 May 2023 12:03:21 +1000 Subject: [PATCH 1/5] Refactor of JASC-PAL Simplified the line-reading function to use `fgets()` which handles windows/unix line endings internally. Simplified the main palette-reading function, and consolidated all the error checking logic in it. I added checks for as many errors I could think of, but happy to add more checks if anyone has suggestions. Thanks to my friend Lito, who knows C much better than me, and workshopped the bones of this refactor with me. --- tools/gbagfx/jasc_pal.c | 168 ++++++++++++---------------------------- 1 file changed, 50 insertions(+), 118 deletions(-) diff --git a/tools/gbagfx/jasc_pal.c b/tools/gbagfx/jasc_pal.c index e5ba9c3c2f..04014b3141 100644 --- a/tools/gbagfx/jasc_pal.c +++ b/tools/gbagfx/jasc_pal.c @@ -2,155 +2,87 @@ #include #include +#include #include "global.h" #include "gfx.h" #include "util.h" -// Read/write Paint Shop Pro palette files. +// Read/write JASC palette files. -// Format of a Paint Shop Pro palette file, line by line: -// "JASC-PAL\r\n" (signature) -// "0100\r\n" (version; seems to always be "0100") -// "\r\n" (number of colors in decimal) +// Format of a JASC palette file, line by line: +// "JASC-PAL" (signature) +// "0100" (version; seems to always be "0100") +// "" (number of colors in decimal) // // times: -// " \r\n" (color entry) +// " " (color entry) +// +// Line endings can be \r\n or \n // // Each color component is a decimal number from 0 to 255. // Examples: -// Black - "0 0 0\r\n" -// Blue - "0 0 255\r\n" -// Brown - "150 75 0\r\n" +// Black - "0 0 0" +// Blue - "0 0 255" +// Brown - "150 75 0" +// White - "255 255 255 255" +// ^~~ ignored -#define MAX_LINE_LENGTH 11 +// 15 chars of color info, \r\n or \n, and \0 +#define MAX_LINE_LENGTH 18 -void ReadJascPaletteLine(FILE *fp, char *line) +int ReadJascPaletteLine(FILE *fp, char *line) { - int c; - int length = 0; - - for (;;) - { - c = fgetc(fp); - - if (c == '\r') - { - c = fgetc(fp); - - if (c != '\n') - FATAL_ERROR("CR line endings aren't supported.\n"); - - line[length] = 0; - - return; - } - - if (c == '\n') - FATAL_ERROR("LF line endings aren't supported.\n"); - - if (c == EOF) - FATAL_ERROR("Unexpected EOF. No CRLF at end of file.\n"); - - if (c == 0) - FATAL_ERROR("NUL character in file.\n"); - - if (length == MAX_LINE_LENGTH) - { - line[length] = 0; - FATAL_ERROR("The line \"%s\" is too long.\n", line); - } - - line[length++] = c; - } + // Read line up to first newline char (inclusive) or [MAX_LINE_LENGTH-1] + if(fgets(line, MAX_LINE_LENGTH, fp) == NULL) + return 0; + line[strcspn(line, "\r\n")] = 0; // Terminate the line at the first newline char + return 1; } void ReadJascPalette(char *path, struct Palette *palette) { - char line[MAX_LINE_LENGTH + 1]; - - FILE *fp = fopen(path, "rb"); - - if (fp == NULL) - FATAL_ERROR("Failed to open JASC-PAL file \"%s\" for reading.\n", path); + char line_buffer[MAX_LINE_LENGTH]; + int red, green, blue; + int numColors; - ReadJascPaletteLine(fp, line); + FILE *fp = fopen(path, "r"); - if (strcmp(line, "JASC-PAL") != 0) - FATAL_ERROR("Invalid JASC-PAL signature.\n"); + if (!fp) + FATAL_ERROR("Cannot open JASC-PAL file \"%s\" with error: %s\n", path, strerror(errno)); - ReadJascPaletteLine(fp, line); + // Check JASC-PAL Header + ReadJascPaletteLine(fp, line_buffer); + if (strcmp(line_buffer, "JASC-PAL") != 0) + FATAL_ERROR("Invalid signature, expected \"JASC-PAL\", read: \"%s\"\n",line_buffer); - if (strcmp(line, "0100") != 0) + ReadJascPaletteLine(fp, line_buffer); + if (strcmp(line_buffer, "0100") != 0) FATAL_ERROR("Unsuported JASC-PAL version.\n"); - ReadJascPaletteLine(fp, line); + // Get number of colors in palette + ReadJascPaletteLine(fp, line_buffer); + numColors = strtol(line_buffer, NULL, 10); - if (!ParseNumber(line, NULL, 10, &palette->numColors)) - FATAL_ERROR("Failed to parse number of colors.\n"); - - if (palette->numColors < 1 || palette->numColors > 256) - FATAL_ERROR("%d is an invalid number of colors. The number of colors must be in the range [1, 256].\n", palette->numColors); - - for (int i = 0; i < palette->numColors; i++) + // Check for sensible number of colors + if (numColors > 0 && numColors <= 256) { - ReadJascPaletteLine(fp, line); - - char *s = line; - char *end; - - int red; - int green; - int blue; - - if (!ParseNumber(s, &end, 10, &red)) - FATAL_ERROR("Failed to parse red color component.\n"); - - s = end; - - if (*s != ' ') - FATAL_ERROR("Expected a space after red color component.\n"); - - s++; - - if (*s < '0' || *s > '9') - FATAL_ERROR("Expected only a space between red and green color components.\n"); - - if (!ParseNumber(s, &end, 10, &green)) - FATAL_ERROR("Failed to parse green color component.\n"); - - s = end; - - if (*s != ' ') - FATAL_ERROR("Expected a space after green color component.\n"); - - s++; - - if (*s < '0' || *s > '9') - FATAL_ERROR("Expected only a space between green and blue color components.\n"); - - if (!ParseNumber(s, &end, 10, &blue)) - FATAL_ERROR("Failed to parse blue color component.\n"); - - if (*end != 0) - FATAL_ERROR("Garbage after blue color component.\n"); - - if (red < 0 || red > 255) - FATAL_ERROR("Red color component (%d) is outside the range [0, 255].\n", red); - - if (green < 0 || green > 255) - FATAL_ERROR("Green color component (%d) is outside the range [0, 255].\n", green); + palette->numColors = numColors; + } else { + FATAL_ERROR("Out-of-Bounds number of colors specifed: %i. Maximum supported is 256\n", numColors); + } - if (blue < 0 || blue > 255) - FATAL_ERROR("Blue color component (%d) is outside the range [0, 255].\n", blue); + // Get color entries + for (int i = 0; i < numColors; ++i) + { + if (ReadJascPaletteLine(fp, line_buffer) == 0) + FATAL_ERROR("Failed to read color index %i\n", i); + if (sscanf(line_buffer, "%d %d %d", &red, &green, &blue) != 3) + FATAL_ERROR("Invalid color format in color \"%s\"\n", line_buffer); palette->colors[i].red = red; palette->colors[i].green = green; palette->colors[i].blue = blue; } - - if (fgetc(fp) != EOF) - FATAL_ERROR("Garbage after color data.\n"); - fclose(fp); } From faae6f19374710c47b549d6d3629bab4994aca6a Mon Sep 17 00:00:00 2001 From: waterwheels Date: Tue, 9 May 2023 15:38:04 +1000 Subject: [PATCH 2/5] Integrate fixes suggested by GriffinRichards Colour number parsing reverted to use `ParseNumber()` to handle long to int conversion and malformed line errors Colour number error message reverted to specify minimum value (1) Colour channels checked for being in-bounds before assignment --- tools/gbagfx/jasc_pal.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/tools/gbagfx/jasc_pal.c b/tools/gbagfx/jasc_pal.c index 04014b3141..eb35a05f72 100644 --- a/tools/gbagfx/jasc_pal.c +++ b/tools/gbagfx/jasc_pal.c @@ -61,14 +61,15 @@ void ReadJascPalette(char *path, struct Palette *palette) // Get number of colors in palette ReadJascPaletteLine(fp, line_buffer); - numColors = strtol(line_buffer, NULL, 10); + if (!ParseNumber(line_buffer, NULL, 10, &numColors)) + FATAL_ERROR("Failed to parse number of colours.\n"); // Check for sensible number of colors if (numColors > 0 && numColors <= 256) { palette->numColors = numColors; } else { - FATAL_ERROR("Out-of-Bounds number of colors specifed: %i. Maximum supported is 256\n", numColors); + FATAL_ERROR("%i is an invalid number of colours. The number of colours must be in the range [0, 256]\n", numColors); } // Get color entries @@ -79,6 +80,13 @@ void ReadJascPalette(char *path, struct Palette *palette) if (sscanf(line_buffer, "%d %d %d", &red, &green, &blue) != 3) FATAL_ERROR("Invalid color format in color \"%s\"\n", line_buffer); + if (red < 0 || red > 255) + FATAL_ERROR("Red color component %d is invalid. Accepted range is [0, 255]", red); + if (green < 0 || green > 255) + FATAL_ERROR("Green color component %d is invalid. Accepted range is [0, 255]", green); + if (blue < 0 || blue > 255) + FATAL_ERROR("Blue color component %d is invalid. Accepted range is [0, 255]", blue); + palette->colors[i].red = red; palette->colors[i].green = green; palette->colors[i].blue = blue; From ba8f1d0541f6ad4b72ce34d19c92566b312ced54 Mon Sep 17 00:00:00 2001 From: waterwheels Date: Sun, 21 May 2023 13:14:58 +1000 Subject: [PATCH 3/5] Amended typos Fixed range on number of colours error message to [1, 256] Added newlines to colour component out-of-range errors Co-Authored-By: Sophia <73026338+sophxm@users.noreply.github.com> --- tools/gbagfx/jasc_pal.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tools/gbagfx/jasc_pal.c b/tools/gbagfx/jasc_pal.c index eb35a05f72..2981616aae 100644 --- a/tools/gbagfx/jasc_pal.c +++ b/tools/gbagfx/jasc_pal.c @@ -69,7 +69,7 @@ void ReadJascPalette(char *path, struct Palette *palette) { palette->numColors = numColors; } else { - FATAL_ERROR("%i is an invalid number of colours. The number of colours must be in the range [0, 256]\n", numColors); + FATAL_ERROR("%i is an invalid number of colours. The number of colours must be in the range [1, 256]\n", numColors); } // Get color entries @@ -81,11 +81,11 @@ void ReadJascPalette(char *path, struct Palette *palette) FATAL_ERROR("Invalid color format in color \"%s\"\n", line_buffer); if (red < 0 || red > 255) - FATAL_ERROR("Red color component %d is invalid. Accepted range is [0, 255]", red); + FATAL_ERROR("Red color component %d is invalid. Accepted range is [0, 255]\n", red); if (green < 0 || green > 255) - FATAL_ERROR("Green color component %d is invalid. Accepted range is [0, 255]", green); + FATAL_ERROR("Green color component %d is invalid. Accepted range is [0, 255]\n", green); if (blue < 0 || blue > 255) - FATAL_ERROR("Blue color component %d is invalid. Accepted range is [0, 255]", blue); + FATAL_ERROR("Blue color component %d is invalid. Accepted range is [0, 255]\n", blue); palette->colors[i].red = red; palette->colors[i].green = green; From 44ef968df06f2887eebe3f7657013799dff9323c Mon Sep 17 00:00:00 2001 From: waterwheels Date: Mon, 22 May 2023 12:02:19 +1000 Subject: [PATCH 4/5] ReadJascPaletteLine() checks it found end of line Fixed error where `\r` in a windows line-ending palette could fit in line buffer but the following `\n` would exceed it and cause the next line read to be empty. Func now checks what index the first newline char is found at, and if it's greater than the max length - 3 (to account for `\r\n\0`) throws a line too long error. --- tools/gbagfx/jasc_pal.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/tools/gbagfx/jasc_pal.c b/tools/gbagfx/jasc_pal.c index 2981616aae..3c7d853a0a 100644 --- a/tools/gbagfx/jasc_pal.c +++ b/tools/gbagfx/jasc_pal.c @@ -32,10 +32,22 @@ int ReadJascPaletteLine(FILE *fp, char *line) { + int end = 0; + // Read line up to first newline char (inclusive) or [MAX_LINE_LENGTH-1] if(fgets(line, MAX_LINE_LENGTH, fp) == NULL) return 0; - line[strcspn(line, "\r\n")] = 0; // Terminate the line at the first newline char + + end = strcspn(line, "\r\n"); // Find index of first newline. + + // Max length of 4-channel colour is 15 chars, so max length of resulting + // string should be 16 including \0. If end-of-line is > 15, there's a chance + // we've left newline chars unread for the next line, so we should error out + if (end > MAX_LINE_LENGTH -3) { + FATAL_ERROR("Line is too long: %s\n", line); + } else { + line[end] = 0; // Terminate the line at the first newline char + } return 1; } From 220a6606fb9b4813faf1341155986c43cc59c3b5 Mon Sep 17 00:00:00 2001 From: waterwheels Date: Mon, 22 May 2023 12:21:27 +1000 Subject: [PATCH 5/5] Added additional error checking Added checks to successful line reading of JASC-PAL, version code, and num colours lines. Additionally added a check that the number of colours line is max 3 chars long (excluding `\0`) to make sure we're not just parsing the first colour channel Co-Authored-By: Sophia <73026338+sophxm@users.noreply.github.com> --- tools/gbagfx/jasc_pal.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/tools/gbagfx/jasc_pal.c b/tools/gbagfx/jasc_pal.c index 3c7d853a0a..8f084f7e6d 100644 --- a/tools/gbagfx/jasc_pal.c +++ b/tools/gbagfx/jasc_pal.c @@ -63,17 +63,20 @@ void ReadJascPalette(char *path, struct Palette *palette) FATAL_ERROR("Cannot open JASC-PAL file \"%s\" with error: %s\n", path, strerror(errno)); // Check JASC-PAL Header - ReadJascPaletteLine(fp, line_buffer); + if (ReadJascPaletteLine(fp, line_buffer) == 0) + FATAL_ERROR("Failed to read JASC-PAL header.\n"); if (strcmp(line_buffer, "JASC-PAL") != 0) FATAL_ERROR("Invalid signature, expected \"JASC-PAL\", read: \"%s\"\n",line_buffer); - ReadJascPaletteLine(fp, line_buffer); + if (ReadJascPaletteLine(fp, line_buffer) == 0) + FATAL_ERROR("Failed to read version.\n"); if (strcmp(line_buffer, "0100") != 0) FATAL_ERROR("Unsuported JASC-PAL version.\n"); // Get number of colors in palette - ReadJascPaletteLine(fp, line_buffer); - if (!ParseNumber(line_buffer, NULL, 10, &numColors)) + if (ReadJascPaletteLine(fp, line_buffer) == 0) + FATAL_ERROR("Failed to read number of colors.\n"); + if (strlen(line_buffer) > 3 || !ParseNumber(line_buffer, NULL, 10, &numColors)) FATAL_ERROR("Failed to parse number of colours.\n"); // Check for sensible number of colors