[Pkg-gmagick-im-team] Bug#846385: imagemagick: Potential ABI break upstream (without SONAME change)

[Simon McVittie]

Control: reopen 846385
Control: severity 846385 serious

tl;dr: yes, this is clearly an ABI break.

On Thu, 01 Dec 2016 at 15:55:02 +0100, roucaries bastien wrote:
> * struct _DrawInfo (1) is not a problem from a C point of view because
> it should be set and destry by API function. It is a opaque object. So
> no need to so bump for this
> *  _ElementReference (1) is part of _Drawinfo so not a problem
> * _GradientInfo (3) is the same
> * For _image is an opaque type so it is the same

Unfortunately this is not true: none of these types are opaque. I think
you have misunderstood what it means to say a struct type is opaque.
For example, DrawInfo (aka struct _DrawInfo) has its layout visible to
library users in the installed header <magick/draw.h> (in
libmagickcore-6-headers), so any change to its size is an ABI break, and
so is any semantic change to its contents.

You are right to think that if library users were expected to allocate
a DrawInfo on the stack, it would certainly be an ABI break to change
its size. However, even if a structure is only ever allocated by
library code, reading its fields directly is part of the library ABI too.

For DrawInfo to be an opaque object, its layout would have to be
in a .c or .h file used only internally by ImageMagick (and not installed
in libmagickcore-6-headers), and library users would have to access its
fields via accessor functions like DrawInfoGetGravity() and
DrawInfoSetGravity(). For example, GRegex in GLib's glib/gregex.[ch]
is a good example of an opaque object:
https://sources.debian.net/src/glib2.0/2.50.2-2/glib/gregex.h/
https://sources.debian.net/src/glib2.0/2.50.2-2/glib/gregex.c/

The definition of an ABI break is that previously-correct code, compiled
against version A, no longer works correctly when linked at runtime with
version B. Consider what would happen if some program that uses ImageMagick
is compiled against ImageMagick 6.9.1-10, and it wants to set the stroke
opacity (which happens to be the last field in the struct):

    DrawInfo *draw_info = AcquireDrawInfo();

    draw_info->stroke_opacity = 0.5;

In ImageMagick 6.9.1-10 on x86_64, according to the ABI Compliance Checker,
the DrawInfo is a 720 byte struct. A double is 8 bytes, and stroke_opacity
is the last thing in the struct, so that assignment results in an 8-byte
write 712 bytes after the draw_info pointer, overwriting the bytes from
712 to 719 bytes after the pointer. In other words, on x86_64, it's the
same machine code that you would get by compiling this:

    DrawInfo *draw_info = AcquireDrawInfo();

    *((double *) (((char *) draw_info) + 712)) = 0.5;

Now suppose we install that program on a machine that has ImageMagick
6.9.2-10, in which DrawInfo has grown by 32 bytes. The program still
writes at an offset of 712 bytes into the struct (remember that
machine code doesn't know anything about structs or names, only
pointers and offsets), but now the larger GradientInfo and
ElementReference have pushed all the later struct fields further
away from offset 0. If my arithmetic is correct, the field 712 bytes
into the data structure is now draw_info->interword_spacing, which is
not what was intended.

Conversely, if you compile against 6.9.2-10 but use 6.9.1-10 at runtime,
it's worse: the program writes at an offset of 724 bytes (overwriting
bytes 724 to 731 inclusive); but since ImageMagick 6.9.1-10 only allocated
a 720 byte struct, that write is outside the struct, and could be anything
(in particular, it could be corrupting glibc malloc data structures).

Regards,
    S