623796 - nsPNGDecoder.cpp:642:44: warning: passing NULL to non-pointer argument 2 of 'void MOZ_PNG_set_crc_action(png_struct*, int, int)'

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Daniel Holbert [:dholbert]]

When building mozilla-central, I get this build warning:

> modules/libpr0n/decoders/nsPNGDecoder.cpp: In static member function 'static void mozilla::imagelib::nsPNGDecoder::info_callback(png_struct*, png_info*)':
> modules/libpr0n/decoders/nsPNGDecoder.cpp:642:44: warning: passing NULL to non-pointer argument 2 of 'void MOZ_PNG_set_crc_action(png_struct*, int, int)'

The chunk of code involved is:
===
>637   /* Reject any ancillary chunk after IDAT with a bad CRC (bug #397593).
>638    * It would be better to show the default frame (if one has already been
>639    * successfully decoded) before bailing, but it's simpler to just bail
>640    * out with an error message.
>641    */
>642   png_set_crc_action(png_ptr, NULL, PNG_CRC_ERROR_QUIT);
===
and it was added in bug 397593, as noted in the comment.

Presumably this was just a typo & we should be passing 0 instead of NULL as the second argument there...?
(or did the called function used to take a pointer whereas now it takes an integer?)

Comment 1

[Daniel Holbert [:dholbert]]

FWIW, this appears to be the only call to png_set_crc_action in all of mozilla-central, based on this search:
http://mxr.mozilla.org/mozilla-central/search?string=png_set_crc_action

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to comment #0)
> Presumably this was just a typo & we should be passing 0 instead of NULL as the
> second argument there...?

or rather, presumably we should be passing PNG_CRC_DEFAULT, the PNG flag that corresponds to a "0"/NULL value in png.h...?

That is, assuming we want the crit_action==PNG_CRC_DEFAULT behavior in png_set_crc_action (which is the behavior that NULL is giving us right now):
>34    switch (crit_action)
...
>55       case PNG_CRC_DEFAULT:
>56       default:
>57          png_ptr->flags &= ~PNG_FLAG_CRC_CRITICAL_MASK;
>58          break;

Comment 3

[Glenn Randers-Pehrson]

crit_action is an int.  The one we really want is PNG_CRC_NO_CHANGE, which
happens to be 5 (defined in png.h).

so: png_set_crc_action(png_ptr, PNG_CRC_NO_CHANGE, PNG_CRC_ERROR_QUIT);

since we have never changed the action from default, this would just leave it so,
without resetting the flag.

Comment 4

[Daniel Holbert [:dholbert]]

(I'm assuming that your NEW --> ASSIGNED bug-status change indicated that you meant to assign it to yourself, so I'm adding you as the assignee.  Feel free to revert if that's not correct.)

Comment 5

[Glenn Randers-Pehrson]

Yes that's what I intended.  I'll prepare the (trivial) patch shortly.

Comment 6

[Glenn Randers-Pehrson]

Attachment: v00: Change NULL to PNG_CRC_NO_CHANGE

Comment 7

[(no longer active)]

This needs approval before it can land.

Comment 8

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/27e81aa25025