Created attachment 343092 [details] [review]
io-ico: Fix option parsing to be locale-independent

sscanf() and strtol() are both locale-dependent. In addition, the return
value of sscanf() was not being checked (so it could fail without being
noticed), and there was no bounds checking being performed.

Bounds checking is now performed, although the bounds have been chosen
for conservative backwards-compatibility, and may not be the most
appropriate.

Coverity CID: 1388522

Created attachment 343093 [details] [review]
io-xpm: Handle fseek() failure

If fseek() fails, the image is corrupt, and so the call to this get_buf
function should fail.

Coverity CID: 1388523

Created attachment 343094 [details] [review]
thumbnailer: Add some assertions to help Coverity

From looking at the code, these assertions should always hold already,
but it clarifies the code and helps static analysis if we add them
explicitly.

(n_pixels == 0) is only true if (s_x1 == s_x2 || s_y1 == s_y2), which is
only true if (dx == 0 || dy == 0). This cannot be true, because dx and
dy are the result of dividing source_[width|height] by
dest_[width|height], and there’s already an assertion that the former
are greater or equal to the latter.

Coverity CID: 1388528

Created attachment 343095 [details] [review]
gdk-pixbuf-loader: Add a missing NULL check

The code just above checks whether image_module is NULL, and it doesn’t
look like it can change in the meantime, so we should also check before
dereferencing it for load_increment.

Coverity CID: 1388529

Review of attachment 343092 [details] [review]:

::: gdk-pixbuf/io-ico.c
@@ +1239,3 @@
+ * @minimum and @maximum are valid inclusively. */
+static gboolean
+ascii_strtoll (const gchar  *str,

We have the same strtol{l,} usage in other loaders. Can you please move that to gdk-pixbuf-private.h, in a separate patch?

Review of attachment 343093 [details] [review]:

Definitely.

Review of attachment 343095 [details] [review]:

> and it doesn’t look like it can change in the meantime

I don't understand the point here. Is this just so that Coverity isn't confused? I'd add an assert(priv->image_module) instead.

Review of attachment 343094 [details] [review]:

Pushed to gnome-thumbnailer-skeleton, can you please fold this into the "Update from gnome-thumbnailer-skeleton" patch?

Review of attachment 343095 [details] [review]:

> I don't understand the point here. Is this just so that Coverity isn't confused? I'd add an assert(priv->image_module) instead.

It was a toss-up between adding this non-fatal check and adding a fatal assertion. Basically, I’m not confident enough about what that code is trying to achieve to be 100% sure that priv->image_module will be set in the mean time, from somewhere. Unless you’re 100% sure that priv->image_module will always be non-NULL at that point, or we rewrite the code to be less all-over-the-place, I’d suggest a non-fatal check rather than an assertion.

Review of attachment 343094 [details] [review]:

> Pushed to gnome-thumbnailer-skeleton, can you please fold this into the "Update from gnome-thumbnailer-skeleton" patch?

Oops, I’ve already pushed that one. I’ll reword the commit message on this one to be “Update from g-t-s”

(In reply to Philip Withnall from comment #9)
> Review of attachment 343095 [details] [review] [review]:
> 
> > I don't understand the point here. Is this just so that Coverity isn't confused? I'd add an assert(priv->image_module) instead.
> 
> It was a toss-up between adding this non-fatal check and adding a fatal
> assertion. Basically, I’m not confident enough about what that code is
> trying to achieve to be 100% sure that priv->image_module will be set in the
> mean time, from somewhere. Unless you’re 100% sure that priv->image_module
> will always be non-NULL at that point, or we rewrite the code to be less
> all-over-the-place, I’d suggest a non-fatal check rather than an assertion.

We're still in development. Please make it fatal, and we'll change it if we see the problem happen in actual usage.

Attachment 343093 [details] pushed as 3a53fc4 - io-xpm: Handle fseek() failure

(In reply to Bastien Nocera from comment #11)
> (In reply to Philip Withnall from comment #9)
> > Review of attachment 343095 [details] [review] [review] [review]:
> > 
> > > I don't understand the point here. Is this just so that Coverity isn't confused? I'd add an assert(priv->image_module) instead.
> > 
> > It was a toss-up between adding this non-fatal check and adding a fatal
> > assertion. Basically, I’m not confident enough about what that code is
> > trying to achieve to be 100% sure that priv->image_module will be set in the
> > mean time, from somewhere. Unless you’re 100% sure that priv->image_module
> > will always be non-NULL at that point, or we rewrite the code to be less
> > all-over-the-place, I’d suggest a non-fatal check rather than an assertion.
> 
> We're still in development. Please make it fatal, and we'll change it if we
> see the problem happen in actual usage.

Makes sense.

Comment on attachment 343095 [details] [review]
gdk-pixbuf-loader: Add a missing NULL check

Attachment 343095 [details] pushed as 2634506 - gdk-pixbuf-loader: Add a missing NULL check

Created attachment 345099 [details] [review]
tests: Check that pixbufs are valid before comparing them

Assert that the pixbufs have non-negative dimensions before comparing
them and potentially using one of the dimensions as a loop bound.

Coverity IDs: 1399711, 1399712

Review of attachment 343092 [details] [review]:

::: gdk-pixbuf/io-ico.c
@@ +1239,3 @@
+ * @minimum and @maximum are valid inclusively. */
+static gboolean
+ascii_strtoll (const gchar  *str,

That sounds sensible, but actually all the other loaders already implement the necessary errno and bounds checks on their strtol() calls. In addition, there is no internal shared library for all the loaders, so in order to share this code we would either have to add it as a static inline in gdk-pixbuf-private.h, or create a new internal utils library. Since there are no bugs with this in the other loaders, that’s refactoring work, which I don’t have time to do at the moment.

So I’d suggest pushing this commit in its current form, at least for the moment.

Review of attachment 343092 [details] [review]:

Fine then.

Review of attachment 345099 [details] [review]:

Sure.

Attachment 343092 [details] pushed as 91723d7 - io-ico: Fix option parsing to be locale-independent
Attachment 345099 [details] pushed as 0f53857 - tests: Check that pixbufs are valid before comparing them