Created attachment 357795 [details] [review]
libdocument: Check for integer overflows when creating a thumbnail

The width and height are being used to create a new pixbuf.
They can, potentially, overflow, because the code adds 4 without
checking whether the result can fit in an int.  An invalidly sized
pixbuf will probably have bad effects when data is written to the
pixel buffer.

This change checks whether the signed integer addition overflows.

Unfortunately, we don't have a g_int_checked_add, so we have to check
manually.

Created attachment 357796 [details] [review]
libdocument: Check for integer multiplication overflow when thumbnailing

The width is being multiplied which may overflow the integer.
The result is used in a call to memset to overwrite memory as the number
of bytes to write.  The effect of an overflown integer are probably
not very dramatic as it would cause too short writes.

The compiler has a complaint slightly related to this problem:

ev-document-misc.c:75:52: warning: conversion to ‘size_t {aka long
unsigned int}’ from ‘int’ may change the sign of the result [-Wsign-
conversion]
    memset (data + (rowstride * i) + 4, 0xffffffff, width_r * 4

This change uses the GLib macro for checking whether a multiplication
overflows and returns if it does indeed overflow.
The change also introduces a new compiler warning:

ev-document-misc.c:77:49: warning: conversion to ‘guint64 {aka long
unsigned int}’ from ‘int’ may change the sign of the result [-Wsign-
conversion]
   nbytes_success = g_size_checked_mul (&nbytes, width_r, 4);

I consider this a compiler bug, however, because width_r is being
checked for being positive in line 61, but the compiler
doesn't seem to realise.

I don't think a g_return[_val]_if_fail is right; these need to be hard errors, either via g_assert() or be propagated upwards by returning early from the functions.

I know we were doing this already, but I'm not sure using g_return macros is a good idea for this kind of checks, since the macros can be disabled. I would turns them into normal early returns, including the existing ones.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/evince/issues/818.