pngrutil.c: Delete dead code in png_handle_PLTE() #484
Conversation
| png_chunk_warning(png_ptr, "CRC error"); | ||
| } | ||
| #endif | ||
| png_crc_finish(png_ptr, (png_uint_32) (length - (unsigned int)num * 3)); |
There was a problem hiding this comment.
(1) Please check if the comment block at lines 1012-1016 should be updated.
(2) I assume there is no bug and therefore the unreachable code can be safely deleted. But perhaps there is a bug and the code should not be unreachable. So please check the code I deleted carefully.
Thanks!
There was a problem hiding this comment.
They are both "#ifndef".
Your logic seems to assume the second is "#ifdef"
There was a problem hiding this comment.
I confirm they are both #ifndef. I did not assume the second is #ifdef.
Earlier in the function, at lines 952-958, we have:
#ifndef PNG_READ_OPT_PLTE_SUPPORTED
if (png_ptr->color_type != PNG_COLOR_TYPE_PALETTE)
{
png_crc_finish(png_ptr, length);
return;
}
#endif
Suppose #ifndef PNG_READ_OPT_PLTE_SUPPORTED is true. If we reach here, the png_ptr->color_type == PNG_COLOR_TYPE_PALETTE condition at line 1018 of the original code must be true, so the else if block at lines 1025-1048 of the original code will never be taken.
There was a problem hiding this comment.
I found that the comment block at lines 1012-1016 describes the original code. So the comment block should be deleted (or updated) in this pull request.
|
The early return from |
Delete unreachable code in png_handle_PLTE() related to the
condition #ifndef PNG_READ_OPT_PLTE_SUPPORTED.
Suppose PNG_READ_OPT_PLTE_SUPPORTED is not defined. If
png_ptr->color_type is not PNG_COLOR_TYPE_PALETTE, the function has
returned earlier in the function. So if we reach here,
png_ptr->color_type must be PNG_COLOR_TYPE_PALETTE, therefore the
if-else statement:
if (png_ptr->color_type == PNG_COLOR_TYPE_PALETTE)
{
A;
}
else if (blah) {
B;
}
is equivalent to:
A;
On the other hand, suppose PNG_READ_OPT_PLTE_SUPPORTED is defined. Then
"if (png_ptr->color_type == PNG_COLOR_TYPE_PALETTE)" and the entire
"else if (blah) { B; }" are not compiled, leaving only "A;".
wantehchang
force-pushed
the
remove-unreachable-code
branch
from
b072314
to
aa9e2c5
Compare
|
Are, that helps understanding. Those lines are clearly wrong for the reasons stated in the lines you want to remove; if READ_OPT_PLTE is unset (i.e. "don't read an optional palette") and the PLTE is optional (not a palette PNG) the code as written unconditionally errors out on a CRC error in the PLTE; pretty much the exact opposite of what is wanted. Easiest fix is just to delete those lines. The actual code in there dates back to 1996/7 and it looks like the support for disabling reading of PLTE in non-palette PNGs never worked correctly. @wantehchang since you are using the option to disable the PLTE read do you see any issues if you delete the erroneous lines? I.e. 952..958 |
|
John: Thank you for taking a look. I am not familiar with libpng. (I remember I spotted this dead code while debugging #482.) I am afraid that I don't know the answer to your question. I work on the Chrome web browser. Chrome undefines PNG_READ_OPT_PLTE_SUPPORTED in its third_party/libpng/pnglibconf.h file. I don't know why. |
|
萬德 if my analysis is correct it's a bug anywhere. Since you are reporting bugs in Chrome: https://chromium-review.googlesource.com/c/chromium/src/+/4616010 I suggest you report the spurious/dangerous exit before the error handling there. They should be able to test it and verify or refute my suggested change. |
|
John: I just wrote a test Chrome changelist (patch) of your suggested change: I sent the changelist to Chrome's commit queue dry run. I will report back if it passes Chrome's test suite. Did you suggest I report this bug in Chrome issue tracker? I can certainly do that, but isn't this a libpng bug? Or did you mean Chrome should not build libpng with PNG_READ_OPT_PLTE_SUPPORTED undefined? |
Yes, that's what I wanted. A full build test of libpng will, I'm fairly sure, just pass simply because it does not unset the option; so it proves nothing.
It certainly looks like a bug in libpng; READ_OPT_PLTE isn't working correctly so far as I can see. I doubt it's an issue in Chrome because I believe Chrome turns off the CRC checking; that's a runtime option, "png_set_crc_action". Unfortunately the tests seem to be failing and they are failing in precisely the area of change. |
|
Oh, it's the other bug you mentioned. Here's one of the results: So, in fact, removing the erroneous code has actually fixed the other bug, but your tests are expecting the unfixed result! Hah. Definitely an issue for Chrome :-) Seems like you will need to fix the tests first (it's not a very good test to test for the wrong answer :-) EDIT: no, not necessarily; it's not clear if the Chrome tests ran with the fix for #482 |
Delete unreachable code in png_handle_PLTE() related to the condition #ifndef PNG_READ_OPT_PLTE_SUPPORTED.
Suppose PNG_READ_OPT_PLTE_SUPPORTED is not defined. If png_ptr->color_type is not PNG_COLOR_TYPE_PALETTE, the function has returned earlier in the function. So if we reach here, png_ptr->color_type must be PNG_COLOR_TYPE_PALETTE, therefore the if-else statement:
is equivalent to:
On the other hand, suppose PNG_READ_OPT_PLTE_SUPPORTED is defined. Then "if (png_ptr->color_type == PNG_COLOR_TYPE_PALETTE)" and the entire "else if (blah) { B; }" are not compiled, leaving only "A;".