IDAT: invalid distance too far back #856

Closed
lpis opened this Issue Apr 4, 2016 · 11 comments

Projects

None yet

4 participants

@lpis
lpis commented Apr 4, 2016

Name of the game:Nepheshel

Player platform (e.g. Windows, Linux, Android, ...):Android

Attach files (as a .zip archive or link them):
Nepheshel203.zip

  • A savegame next to the problem (if it isn't at the beginning of the game)
  • The easyrpg_log.txt logfile (you can find it in the game directory)

Describe the issue in detail and how to reproduce it:
screenshot_2016-03-17-12-28-16

As soon as the file opened, this error (says "IDAT: invalid distance too far back") comes up and never work

@Ghabry Ghabry added the Bitmaps label Apr 4, 2016
@Ghabry
Member
Ghabry commented Apr 4, 2016

Thanks. This looks like a problem with our PNG image parser. We will investigate.

To Devs:
libpng readme https://github.com/glennrp/libpng/blob/edad4639cf1a8fe4e5ecf7631815906de051fa6d/libpng-manual.txt#L5122

png_set_option(png_ptr, PNG_MAXIMUM_INFLATE_WINDOW, PNG_OPTION_ON);
could work

@fdelapena fdelapena added the Crash label Apr 4, 2016
@fdelapena fdelapena added this to the 0.5.0 milestone Apr 4, 2016
@fdelapena
Member

I've tested the attached game on desktop and loads PNG images properly. There is a known bug at least between libpng 1.6.0 and 1.6.3 showing this message. Maybe the libpng version used in the toolchain is outdated.

@Ghabry
Member
Ghabry commented Apr 4, 2016

I can reproduce the bug under Windows (same Version as libpng on Android)

@Ghabry
Member
Ghabry commented Apr 4, 2016

And we can't really fix this because libpng calls the error handler and when our error handler returns libpng terminates the application m(

@fdelapena
Member

It works for me with libpng 1.6.18, maybe there is a bug in the most recent version.

@Ghabry
Member
Ghabry commented May 3, 2016 edited

ImagePNG needs some error handling (a bool return) so that the callee can draw a checkerboard e.g.

I just remembered how to work around the libpng Exit:

jmp_buf env;

static void on_png_error(png_structp, png_const_charp error_msg) {
    Output::Warning("%s", error_msg);
    longjmp(env, 1);
}

// beginning of ReadPNG
    int jmp_res = setjmp(env);
    if (jmp_res == 1) {
        return;
    }

\o/

Note that this will memleak on error but because this is error is so seldom I don't think this will really hurt.

@carstene1ns
Member

@lpis: While we unfortunately cannot make the original image work with our used library, it can at least be replaced by yourself locally:
default
The image links to a zip archive containing the fixed image, extract it into Nepheshel203/System overwriting the broken one and you should be able to play the game.


@Ghabry: While I think it is a good idea to draw the checkerboard for all other missing or broken images, using our own system image in this case is better yet. We ship it anyway and the worst thing that could happen is that the game texts/messages looks a bit ugly if the creator changed the colors too much (i.e. very dark vs. our shiny green). But even this is better than having no system image at all (mixed/chopped checkerboards everywhere).

@Ghabry
Member
Ghabry commented May 27, 2016

Yeah, you are right. I forgot about the special case for "System"

@fdelapena
Member
fdelapena commented Jul 13, 2016 edited

It works for me with libpng 1.6.18, maybe there is a bug in the most recent version.

Confirmed it happens here with 1.6.23, also affects libpng-dependent programs, such eog.

Update: it's confirmed as a security related change and this made libpng lesser tolerant with these broken PNG files, so it's a won't fix (glennrp/libpng#125). Unfortunately, all games containing them will need to manual repair these assets with old libpng reading and do a new "save as" from a PNG image editor, as they can't be repaired with pngfix. So, to prevent closing Player it needs implementing error handling to prevent a crash.

@fdelapena
Member
fdelapena commented Jul 13, 2016 edited

Note that this will memleak on error but because this is error is so seldom I don't think this will really hurt.

@Ghabry the setjmp error handling leak could be mitigated by freeing png data if any, nullptr info_ptr if not null and calling png_destroy_read_struct if png_ptr is not null as usual, at least other software do this way in the error handler callback.

LGTM for the crash workaround and also a solution for this particular game, as currently there is an embedded fallback system graphics file as carstene1ns points 👍.

@carstene1ns
Member

The double jump @Ghabry proposes is not necessary. Combined with @fdelapena's idea this will likely be memleak free.

@fdelapena fdelapena modified the milestone: 0.4.2, 0.5.0 Jul 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment