#2812 Incorrect ARGB interpretation of color column in splot's boxes and polygons styles

[Hiroki Motoyoshi]

There is a bug when using splot with the boxes or polygon style: the ARGB value read from the color column may be misinterpreted, resulting in incorrect rendering of transparent colors.

The color column value is stored as a double (CRD_COLOR), but improper casting between int, unsigned int, and double can cause overflow or underflow for ARGB value (32bit integer):

Overflow: unsigned int → double → int
Underflow: int → double → unsigned int

I have created a patch to address this issue. Test scripts "example_boxes.gp" and "example_polygon.gp" are also attached for verification.

[Ethan Merritt]

Another reminder why I really want to refactor the code so that fill color is kept separately from line properties.

The proposed fix for 3D boxes is more complicated than necessary. For a simpler immediate fix it is sufficient to cast to (unsigned)
plot->lp_properties.pm3d_color.lt = (unsigned) points[i].CRD_COLOR;

The problem with polygons lies elsewhere. It is due to an incorrect declaration (my fault) in pm3d_add_polygon.

diff --git a/src/pm3d.c b/src/pm3d.c
index e9325f0d8..944cc7a98 100644
--- a/src/pm3d.c
+++ b/src/pm3d.c
@@ -1273,7 +1273,7 @@ pm3d_add_polygon(struct surface_points *plot, gpdPoint corners[], int vertices)
     } else if (plot->plot_style == ISOSURFACE
            ||  plot->plot_style == POLYGONS) {

-    int rgb_color = corners[0].c;
+    unsigned rgb_color = corners[0].c;
     if (corners[0].c == LT_BACKGROUND)
         q->gray = PM3D_USE_BACKGROUND_INSTEAD_OF_GRAY;
     else

I think these fixes are OK for both version 6.0 and 6.1

Possible improvement for the development version going forward

It is tempting to separate 32 bit unsigned ARGB color from signed linetype color at the level of the t_colorspec structure. See attached patchset. But I worry that this approach puts additional requirements on what C compiler support is required to build gnuplot. Version 6 already bumped the minimum from c89 to c99. Requiring support for anonymous unions, as in the attached patchset, bumps it further to C11 for strict standards-compliance. On the other hand this may not be a problem in practice, since anonymous unions have been accepted in gcc and clang for a long time. So maybe it's OK? I think I will try to get some discussion about this on the mailing list.

[Hiroki Motoyoshi]

You may find this hard to believe, but the following approach does not work as expected on ARM-based systems:

plot->lp_properties.pm3d_color.lt = (unsigned) points[i].CRD_COLOR;

If points[i].CRD_COLOR holds a negative value, a round-toward-zero conversion may occur on ARM-based systems. For example, if the color value #8000ff00 (which is -2147418368 as a 32-bit signed int) is stored in CRD_COLOR as -2147418368.0, casting it to a 32-bit unsigned int may result in 0 on ARM-based systems!

This behavior aligns with known ARM-specific differences in type casting semantics.
For more details, see the section “Conversion of floating-point to unsigned integer” in Microsoft’s documentation on Common Visual C++ ARM migration issues. You’ll find many developers expressing confusion about this issue in various places online.

The code I proposed was written with this in mind.

However, I think the root cause lies in the fact that CRD_COLOR can contain negative values for ARGB value. If CRD_COLOR always held positive values (i.e., double values cast from unsigned integers) for ARGB, this issue wouldn’t occur.

Would it be difficult to ensure that CRD_COLOR always contains a positive value for ARGB when it is read?

[Ethan Merritt]

If points[i].CRD_COLOR holds a negative value, a round-toward-zero conversion may occur on ARM-based systems.

But points[i].CRD_COLOR should never hold a negative value if the input was a 32bit ARGB value. In this case it is 64-bit floating point representation of a positive integer 0x8000ff00, i.e. decimal 2147548928.

If I understand correctly the section of Microsoft documentation you point to, this value is not a problem. The documentation talks about what happens "if the floating-point value is outside the range that the integer can represent" and possible truncation to a smaller sized integer (i.e. short or char). But that isn't the case here. The value can be correctly represented as an unsigned 32bit integer, so as I read it no saturation or truncation should happen when it is cast to (unsigned).

After the cast, it is no longer a floating point number and therefore is not in the scope of that documentation section. The question becomes what happens in the simpler case:

   t_colorspec color;
   unsigned rgb = 0x08000ff00;
   colorspec.lt = rgb;

That pattern appears many places in the gnuplot code, so if it fails to preserve all 32 bits on ARM we have bigger problems than this one case of transparent 3D boxes. On the bright side, many [most? all?] of these cases could be modified to store into an explicitly unsigned destination as in the patchset I attached before.

Would it be difficult to ensure that CRD_COLOR always contains a positive value for ARGB when it is read?

So far as I know this is already the case. The input happens in datafile.c:df_tokenise() via clib routine strtod. Reading a 32bit hexadecimal constant should always yield a positive floating-point value.

If I am misunderstanding the Microsoft documentation, then add me to the list of confused developers out there on the web.

[Ethan Merritt]

Still trying to understand exactly where the ARM compiler (or architecture) might be failing to preserve all 32 bits... Does it work to split the assignment into two statements?

    /* Copy variable color value into plot header for pm3d_add_quadrangle */
    if (plot->pm3d_color_from_column) {
        unsigned rgbcolor = points[i].CRD_COLOR;
        plot->lp_properties.pm3d_color.lt = rgbcolor;
    }

[Hiroki Motoyoshi]

Sorry for the confusion.

My patch was based on the assumption (perhaps mistaken) that CRD_COLOR could have negative values when treated as an ARGB value via a signed int. But since it's ensured that CRD_COLOR always holds only positive values within the unsigned int range for ARGB, I think the original approach you suggested will work fine for both boxes and polygons.

There is one bug caused by this ARM compiler issue, so I'll open a separate ticket for that.

[Ethan Merritt]

What compiler and hardware are you using? Do you know if these issues are a concern on the Apple M1/M2/M3 machines?

[Hiroki Motoyoshi]

What compiler and hardware are you using?

I'm using MacOS Sequoia on Apple M2 chip and the compiler is clang included in standard Apple Xcode.

I confirmed that the latest two commits resolved this issue on such that environment.

[Ethan Merritt]

I am afraid there are many places in the code that will potentially run into the same issues.
Some of these may be false positives, but all are at least suspect:

[~/git/gnuplot/src] grep -n ' = .*\.lt;' *.c
graph3d.c:4348:     quad[0].c = plot->lp_properties.pm3d_color.lt;
graphics.c:5260:            quad[0].c = this_object->lp_properties.pm3d_color.lt;
graphics.c:5262:                int base_color = this_object->lp_properties.pm3d_color.lt;
graphics.c:5275:                quad[0].c = face.pm3d_color.lt;
hidden3d.c:1211:                            labelpoint.CRD_COLOR = label->textcolor.lt;
plot3d.c:1278:                  color = lptmp.pm3d_color.lt;
pm3d.c:727:                 cb1 = cb2 = cb3 = cb4 = fillcolorspec.lt;
pm3d.c:735:                     cb1 = cb2 = cb3 = cb4 = (unsigned int) style.pm3d_color.lt;
pm3d.c:962:                         gray = fillcolorspec.lt;
pm3d.c:970:                             gray = style.pm3d_color.lt;
pm3d.c:1252:        q->qcolor = plot->lp_properties.pm3d_color.lt;
pm3d.c:1295:        rgb_color = style.pm3d_color.lt;
vplot.c:325:        quad[0].c = plot->lp_properties.pm3d_color.lt;
vplot.c:362:        quad[0].c = plot->lp_properties.pm3d_color.lt;

[Hiroki Motoyoshi]

The following lines need explict type conversions using '(unsigned int)'.

graph3d.c:4348: quad[0].c = plot->lp_properties.pm3d_color.lt;
graphics.c:5260: quad[0].c = this_object->lp_properties.pm3d_color.lt;
graphics.c:5275: quad[0].c = face.pm3d_color.lt;
plot3d.c:1278: color = lptmp.pm3d_color.lt;
pm3d.c:727: cb1 = cb2 = cb3 = cb4 = fillcolorspec.lt;
pm3d.c:962: gray = fillcolorspec.lt;
pm3d.c:970: gray = style.pm3d_color.lt;
vplot.c:325: quad[0].c = plot->lp_properties.pm3d_color.lt;
vplot.c:362: quad[0].c = plot->lp_properties.pm3d_color.lt;

I can't confirm if the following lines need to be modified because I don't have the right sample script to reproduce them.

hidden3d.c:1211: labelpoint.CRD_COLOR = label->textcolor.lt;

No further changes are necessary for these lines (in my opinion).

graphics.c:5262: int base_color = this_object->lp_properties.pm3d_color.lt;
pm3d.c:735: cb1 = cb2 = cb3 = cb4 = (unsigned int) style.pm3d_color.lt;
pm3d.c:1252: q->qcolor = plot->lp_properties.pm3d_color.lt;
pm3d.c:1295: rgb_color = style.pm3d_color.lt;

[Hiroki Motoyoshi]

Thank you for the series of commits related to the handling of colorspec.

The commit 7879f3e32 does not include the fix for the following lines.

  vplot.c:325:        quad[0].c = plot->lp_properties.pm3d_color.lt;
  vplot.c:362:        quad[0].c = plot->lp_properties.pm3d_color.lt;

A patch for them should be as follows:

diff --git a/src/vplot.c b/src/vplot.c
index e15ecc1aa..253c38969 100644
--- a/src/vplot.c
+++ b/src/vplot.c
@@ -322,7 +322,7 @@ tessellate_one_cube( struct surface_points *plot, int ix, int iy, int iz )
                    quad[3].z = intersection[ivertex][2];

            /* Color choice */
-           quad[0].c = plot->lp_properties.pm3d_color.lt;
+           quad[0].c = plot->lp_properties.pm3d_color.rgbcolor;

            /* Debugging aid: light up all facets of the same class */
            if (debug > 0 && debug == corner_flags)
@@ -359,7 +359,7 @@ tessellate_one_cube( struct surface_points *plot, int ix, int iy, int iz )
                    quad[3] = quad[2];

            /* Color choice */
-           quad[0].c = plot->lp_properties.pm3d_color.lt;
+           quad[0].c = plot->lp_properties.pm3d_color.rgbcolor;

            /* Hand off this triangle to the pm3d code */
            pm3d_add_quadrangle( plot, quad );

[Ethan Merritt]

Thanks. Let me know if you find any other places.