From 15ab46d03ee5dada9f21ad8295e3286acd57c1d1 Mon Sep 17 00:00:00 2001 From: Kim Woelders Date: Tue, 2 Jan 2024 06:25:36 +0100 Subject: [PATCH] savers: Fix error returns The savers did not return meaningful error codes in many cases. --- src/lib/image.c | 6 ++- src/modules/loaders/loader_argb.c | 14 +++++-- src/modules/loaders/loader_bmp.c | 15 ++++++-- src/modules/loaders/loader_ff.c | 4 +- src/modules/loaders/loader_jpeg.c | 6 +-- src/modules/loaders/loader_jxl.c | 2 +- src/modules/loaders/loader_png.c | 10 ++--- src/modules/loaders/loader_pnm.c | 20 +++++++--- src/modules/loaders/loader_tga.c | 11 ++++-- src/modules/loaders/loader_tiff.c | 6 +-- src/modules/loaders/loader_webp.c | 2 +- src/modules/loaders/loader_xbm.c | 22 +++++++---- test/test_save.cpp | 62 +++++++++++++++++++++++++++++++ 13 files changed, 140 insertions(+), 40 deletions(-) diff --git a/src/lib/image.c b/src/lib/image.c index 13e64ed..d42195c 100644 --- a/src/lib/image.c +++ b/src/lib/image.c @@ -478,7 +478,7 @@ __imlib_LoadErrorToErrno(int loader_ret, int save) case LOAD_OOM: return ENOMEM; case LOAD_BADFILE: - return errno; + return errno ? errno : IMLIB_ERR_INTERNAL; case LOAD_BADIMAGE: return IMLIB_ERR_BAD_IMAGE; case LOAD_BADFRAME: @@ -913,7 +913,11 @@ __imlib_SaveImage(ImlibImage *im, const char *file, ImlibLoadArgs *ila) loader_ret = l->module->save(im); if (!ila->fp) + { + if (fflush(im->fi->fp) != 0) + loader_ret = LOAD_BADFILE; /* Use errno */ fclose(fp); + } __imlib_ImageFileContextPop(im); diff --git a/src/modules/loaders/loader_argb.c b/src/modules/loaders/loader_argb.c index 55daffa..070ff45 100644 --- a/src/modules/loaders/loader_argb.c +++ b/src/modules/loaders/loader_argb.c @@ -112,14 +112,20 @@ _save(ImlibImage *im) FILE *f = im->fi->fp; const uint32_t *imdata; int y, alpha = 0; + size_t nw; #ifdef WORDS_BIGENDIAN uint32_t *buf = (uint32_t *) malloc(im->w * 4); + if (!buf) + QUIT_WITH_RC(LOAD_OOM); #endif + rc = LOAD_BADFILE; + alpha = !!im->has_alpha; - fprintf(f, "ARGB %i %i %i\n", im->w, im->h, alpha); + if (fprintf(f, "ARGB %i %i %i\n", im->w, im->h, alpha) <= 0) + goto quit; imdata = im->data; for (y = 0; y < im->h; y++, imdata += im->w) @@ -131,11 +137,13 @@ _save(ImlibImage *im) memcpy(buf, imdata, im->w * 4); for (x = 0; x < im->w; x++) SWAP_LE_32_INPLACE(buf[x]); - fwrite(buf, im->w, 4, f); + nw = fwrite(buf, 4, im->w, f); } #else - fwrite(imdata, im->w, 4, f); + nw = fwrite(imdata, 4, im->w, f); #endif + if (nw != (size_t)im->w) + goto quit; if (im->lc && __imlib_LoadProgressRows(im, y, 1)) QUIT_WITH_RC(LOAD_BREAK); diff --git a/src/modules/loaders/loader_bmp.c b/src/modules/loaders/loader_bmp.c index e10a42b..0ca1798 100644 --- a/src/modules/loaders/loader_bmp.c +++ b/src/modules/loaders/loader_bmp.c @@ -107,7 +107,7 @@ enum { }; static int -WriteleByte(FILE *file, unsigned char val) +_WriteleByte(FILE *file, unsigned char val) { int rc; @@ -119,7 +119,7 @@ WriteleByte(FILE *file, unsigned char val) } static int -WriteleShort(FILE *file, unsigned short val) +_WriteleShort(FILE *file, unsigned short val) { int rc; @@ -134,7 +134,7 @@ WriteleShort(FILE *file, unsigned short val) } static int -WriteleLong(FILE *file, unsigned long val) +_WriteleLong(FILE *file, unsigned long val) { int rc; @@ -755,6 +755,10 @@ _load(ImlibImage *im, int load_data) return rc; } +#define WriteleByte(file, val) if (!_WriteleByte(file, val)) goto quit +#define WriteleShort(file, val) if (!_WriteleShort(file, val)) goto quit +#define WriteleLong(file, val) if (!_WriteleLong(file, val)) goto quit + static int _save(ImlibImage *im) { @@ -763,7 +767,7 @@ _save(ImlibImage *im) int i, j, pad; uint32_t pixel; - rc = LOAD_SUCCESS; + rc = LOAD_BADFILE; /* calculate number of bytes to pad on end of each row */ pad = (4 - ((im->w * 3) % 4)) & 0x03; @@ -800,6 +804,9 @@ _save(ImlibImage *im) WriteleByte(f, 0); } + rc = LOAD_SUCCESS; + + quit: return rc; } diff --git a/src/modules/loaders/loader_ff.c b/src/modules/loaders/loader_ff.c index 6537ba3..1d52acb 100644 --- a/src/modules/loaders/loader_ff.c +++ b/src/modules/loaders/loader_ff.c @@ -90,7 +90,7 @@ _save(ImlibImage *im) uint16_t *row; const uint8_t *imdata; - rc = LOAD_FAIL; + rc = LOAD_BADFILE; row = NULL; /* write header */ @@ -108,7 +108,7 @@ _save(ImlibImage *im) rowlen = im->w * (sizeof("RGBA") - 1); row = malloc(rowlen * sizeof(uint16_t)); if (!row) - goto quit; + QUIT_WITH_RC(LOAD_OOM); imdata = (uint8_t *) im->data; for (i = 0; i < (uint32_t) im->h; ++i, imdata += rowlen) diff --git a/src/modules/loaders/loader_jpeg.c b/src/modules/loaders/loader_jpeg.c index 6cd4d57..df177ca 100644 --- a/src/modules/loaders/loader_jpeg.c +++ b/src/modules/loaders/loader_jpeg.c @@ -268,14 +268,14 @@ _save(ImlibImage *im) /* allocate a small buffer to convert image data */ buf = malloc(im->w * 3 * sizeof(uint8_t)); if (!buf) - return LOAD_FAIL; + return LOAD_OOM; - rc = LOAD_FAIL; + rc = LOAD_BADFILE; /* set up error handling */ jcs.err = _jdata_init(&jdata); if (sigsetjmp(jdata.setjmp_buffer, 1)) - QUIT_WITH_RC(LOAD_FAIL); + QUIT_WITH_RC(LOAD_BADFILE); /* setup compress params */ jpeg_create_compress(&jcs); diff --git a/src/modules/loaders/loader_jxl.c b/src/modules/loaders/loader_jxl.c index c9f0d22..1bf133c 100644 --- a/src/modules/loaders/loader_jxl.c +++ b/src/modules/loaders/loader_jxl.c @@ -240,7 +240,7 @@ _save(ImlibImage *im) JxlParallelRunner *runner = NULL; #endif - rc = LOAD_FAIL; + rc = LOAD_BADFILE; enc = JxlEncoderCreate(NULL); if (!enc) diff --git a/src/modules/loaders/loader_png.c b/src/modules/loaders/loader_png.c index 18a0088..ccc73d6 100644 --- a/src/modules/loaders/loader_png.c +++ b/src/modules/loaders/loader_png.c @@ -615,24 +615,24 @@ _save(ImlibImage *im) { row_ptr = malloc(im->w * 3 * sizeof(png_byte)); if (!row_ptr) - return LOAD_FAIL; + return LOAD_OOM; } row_buf = row_ptr; - rc = LOAD_FAIL; + rc = LOAD_BADFILE; info_ptr = NULL; png_ptr = png_create_write_struct(PNG_LIBPNG_VER_STRING, NULL, user_error_fn, user_warning_fn); if (!png_ptr) - goto quit; + QUIT_WITH_RC(LOAD_OOM); info_ptr = png_create_info_struct(png_ptr); if (!info_ptr) - goto quit; + QUIT_WITH_RC(LOAD_OOM); if (setjmp(png_jmpbuf(png_ptr))) - QUIT_WITH_RC(LOAD_FAIL); + QUIT_WITH_RC(LOAD_BADFILE); /* check whether we should use interlacing */ interlace = PNG_INTERLACE_NONE; diff --git a/src/modules/loaders/loader_pnm.c b/src/modules/loaders/loader_pnm.c index 93401cf..71ec4c0 100644 --- a/src/modules/loaders/loader_pnm.c +++ b/src/modules/loaders/loader_pnm.c @@ -620,19 +620,21 @@ _save(ImlibImage *im) const uint32_t *imdata; int x, y; - rc = LOAD_FAIL; + rc = LOAD_BADFILE; /* allocate a small buffer to convert image data */ buf = malloc(im->w * 4 * sizeof(uint8_t)); if (!buf) - goto quit; + QUIT_WITH_RC(LOAD_OOM); imdata = im->data; /* if the image has a useful alpha channel */ if (im->has_alpha) { - fprintf(f, fmt_rgba, im->w, im->h); + if (fprintf(f, fmt_rgba, im->w, im->h) <= 0) + goto quit; + for (y = 0; y < im->h; y++) { bptr = buf; @@ -646,7 +648,9 @@ _save(ImlibImage *im) bptr[3] = PIXEL_A(pixel); bptr += 4; } - fwrite(buf, im->w * 4, 1, f); + + if (fwrite(buf, 4, im->w, f) != (size_t)im->w) + goto quit; if (im->lc && __imlib_LoadProgressRows(im, y, 1)) goto quit_progress; @@ -654,7 +658,9 @@ _save(ImlibImage *im) } else { - fprintf(f, fmt_rgb, im->w, im->h); + if (fprintf(f, fmt_rgb, im->w, im->h) <= 0) + goto quit; + for (y = 0; y < im->h; y++) { bptr = buf; @@ -667,7 +673,9 @@ _save(ImlibImage *im) bptr[2] = PIXEL_B(pixel); bptr += 3; } - fwrite(buf, im->w * 3, 1, f); + + if (fwrite(buf, 3, im->w, f) != (size_t)im->w) + goto quit; if (im->lc && __imlib_LoadProgressRows(im, y, 1)) goto quit_progress; diff --git a/src/modules/loaders/loader_tga.c b/src/modules/loaders/loader_tga.c index 19a19b2..c47e713 100644 --- a/src/modules/loaders/loader_tga.c +++ b/src/modules/loaders/loader_tga.c @@ -502,7 +502,7 @@ _save(ImlibImage *im) int y; tga_header header; - rc = LOAD_FAIL; + rc = LOAD_BADFILE; /* assemble the TGA header information */ @@ -533,7 +533,7 @@ _save(ImlibImage *im) /* allocate a buffer to receive the BGRA-swapped pixel values */ buf = malloc(im->w * im->h * (im->has_alpha ? 4 : 3)); if (!buf) - goto quit; + QUIT_WITH_RC(LOAD_OOM); /* now we have to read from im->data into buf, swapping RGBA to BGRA */ imdata = im->data; @@ -562,10 +562,13 @@ _save(ImlibImage *im) } /* write the header */ - fwrite(&header, sizeof(header), 1, f); + if (fwrite(&header, 1, sizeof(header), f) != sizeof(header)) + goto quit; /* write the image data */ - fwrite(buf, 1, im->w * im->h * (im->has_alpha ? 4 : 3), f); + if (fwrite(buf, (im->has_alpha ? 4 : 3), im->w * im->h, f) != + (size_t)(im->w * im->h)) + goto quit; rc = LOAD_SUCCESS; diff --git a/src/modules/loaders/loader_tiff.c b/src/modules/loaders/loader_tiff.c index ae71ded..0ea7f7c 100644 --- a/src/modules/loaders/loader_tiff.c +++ b/src/modules/loaders/loader_tiff.c @@ -488,9 +488,9 @@ _save(ImlibImage *im) tif = TIFFFdOpen(fileno(im->fi->fp), im->fi->name, "w"); if (!tif) - return LOAD_FAIL; + return LOAD_BADFILE; - rc = LOAD_FAIL; + rc = LOAD_BADFILE; /* None of the TIFFSetFields are checked for errors, but since they */ /* shouldn't fail, this shouldn't be a problem */ @@ -559,7 +559,7 @@ _save(ImlibImage *im) buf = _TIFFmalloc(TIFFScanlineSize(tif)); if (!buf) - goto quit; + QUIT_WITH_RC(LOAD_OOM); imdata = im->data; for (y = 0; y < im->h; y++) diff --git a/src/modules/loaders/loader_webp.c b/src/modules/loaders/loader_webp.c index ecea187..5eb19af 100644 --- a/src/modules/loaders/loader_webp.c +++ b/src/modules/loaders/loader_webp.c @@ -137,7 +137,7 @@ _save(ImlibImage *im) int lossless; int free_pic = 0; - rc = LOAD_FAIL; + rc = LOAD_BADFILE; if (!WebPConfigInit(&conf) || !WebPPictureInit(&pic)) goto quit; diff --git a/src/modules/loaders/loader_xbm.c b/src/modules/loaders/loader_xbm.c index 3ca53c4..4eb8b8c 100644 --- a/src/modules/loaders/loader_xbm.c +++ b/src/modules/loaders/loader_xbm.c @@ -218,7 +218,7 @@ _save(ImlibImage *im) int i, k, x, y, bits, nval, val; const uint32_t *imdata; - rc = LOAD_SUCCESS; + rc = LOAD_BADFILE; name = im->fi->name; if ((s = strrchr(name, '/')) != 0) @@ -226,9 +226,12 @@ _save(ImlibImage *im) bname = strndup(name, strcspn(name, ".")); - fprintf(f, "#define %s_width %d\n", bname, im->w); - fprintf(f, "#define %s_height %d\n", bname, im->h); - fprintf(f, "static unsigned char %s_bits[] = {\n", bname); + if (fprintf(f, "#define %s_width %d\n", bname, im->w) <= 0) + goto quit; + if (fprintf(f, "#define %s_height %d\n", bname, im->h) <= 0) + goto quit; + if (fprintf(f, "static unsigned char %s_bits[] = {\n", bname) <= 0) + goto quit; free(bname); @@ -251,12 +254,17 @@ _save(ImlibImage *im) } k++; DL("x, y = %2d,%2d: %d/%d\n", x, y, k, nval); - fprintf(f, " 0x%02x%s%s", bits, k < nval ? "," : "", - (k == nval) || ((k % 12) == 0) ? "\n" : ""); + if (fprintf(f, " 0x%02x%s%s", bits, k < nval ? "," : "", + (k == nval) || ((k % 12) == 0) ? "\n" : "") <= 0) + goto quit; } - fprintf(f, "};\n"); + if (fprintf(f, "};\n") <= 0) + goto quit; + rc = LOAD_SUCCESS; + + quit: return rc; } diff --git a/test/test_save.cpp b/test/test_save.cpp index 70908db..d6c55ba 100644 --- a/test/test_save.cpp +++ b/test/test_save.cpp @@ -333,3 +333,65 @@ TEST(SAVE, save_2b_defer) test_save_2("icon-64.gif", "svg", immed, false); test_save_2("icon-64.gif", "png", immed, true, 4016720483); } + +static void +test_save_3(const char *file, const char *dest, int err_new, int err_old) +{ + char filei[256]; + char fileo[256]; + unsigned int i; + const char *ext; + int err; + Imlib_Image im; + Imlib_Load_Error lerr; + + snprintf(filei, sizeof(filei), "%s/%s", IMG_SRC, file); + D("Load '%s'\n", filei); + im = imlib_load_image(filei); + ASSERT_TRUE(im); + + imlib_context_set_image(im); + + for (i = 0; i < N_PFX; i++) + { + ext = exts[i].ext; + + imlib_context_set_image(im); + imlib_image_set_format(ext); + snprintf(fileo, sizeof(fileo), "%s", dest); + pr_info("Save %s to '%s'", ext, fileo); + + imlib_save_image_with_errno_return(fileo, &err); + EXPECT_EQ(err, err_new); + + imlib_save_image_with_error_return(fileo, &lerr); + EXPECT_EQ(lerr, err_old); + + imlib_save_image(fileo); + err = imlib_get_error(); + ASSERT_EQ(err, err_new); + } + + imlib_context_set_image(im); + imlib_free_image_and_decache(); +} + +TEST(SAVE, save_3_full) +{ + test_save_3("icon-64.png", "/dev/full", + ENOSPC, IMLIB_LOAD_ERROR_OUT_OF_DISK_SPACE); +} + +#if 0 +TEST(SAVE, save_3_dir) +{ + test_save_3("icon-64.png", "/tmp", + EISDIR, IMLIB_LOAD_ERROR_FILE_IS_DIRECTORY); +} + +TEST(SAVE, save_3_rdonly) +{ + test_save_3("icon-64.png", IMG_GEN "/rdonly", + EACCES, IMLIB_LOAD_ERROR_PERMISSION_DENIED_TO_WRITE); +} +#endif