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
