[PATCH 0/3] MR4803: dlls/gdiplus: Assorted path changes

[David Kahurani (＠kahurani)]

Signed-off-by: David Kahurani [e-mail]

[PATCH 1/3] dlls/gdiplus: Add helper function for strictly allocating to a path

[David Kahurani]

From: David Kahurani

Serves to strictly allocate the requested size to a path
unlike lengthen_path whose algorithm allocates more than strictly
required

Signed-off-by: David Kahurani 
---
 dlls/gdiplus/gdiplus.c         | 18 ++++++++++++++++++
 dlls/gdiplus/gdiplus_private.h |  1 +
 dlls/gdiplus/graphicspath.c    | 19 ++-----------------
 dlls/gdiplus/metafile.c        | 11 ++---------
 4 files changed, 23 insertions(+), 26 deletions(-)

diff --git a/dlls/gdiplus/gdiplus.c b/dlls/gdiplus/gdiplus.c
index 32e287b0409..020a9de8acc 100644
--- a/dlls/gdiplus/gdiplus.c
+++ b/dlls/gdiplus/gdiplus.c
@@ -442,6 +442,24 @@ BOOL lengthen_path(GpPath *path, INT len)
     return TRUE;
 }

+BOOL lengthen_path_strict(GpPath *path, INT len)
+{
+    /* Strictly allocate the requested memory size */
+    /* Used only on fresh paths */
+    path->pathdata.Points = calloc(len, sizeof(PointF));
+    path->pathdata.Types = calloc(1, len);
+
+    if (!path->pathdata.Points || !path->pathdata.Types)
+    {
+        free(path->pathdata.Points);
+        free(path->pathdata.Types);
+        free(path);
+        return FALSE;
+    }
+
+    return TRUE;
+}
+
 void convert_32bppARGB_to_32bppPARGB(UINT width, UINT height,
     BYTE *dst_bits, INT dst_stride, const BYTE *src_bits, INT src_stride)
 {
diff --git a/dlls/gdiplus/gdiplus_private.h b/dlls/gdiplus/gdiplus_private.h
index 6f7e72124c2..0dae222ed12 100644
--- a/dlls/gdiplus/gdiplus_private.h
+++ b/dlls/gdiplus/gdiplus_private.h
@@ -130,6 +130,7 @@ extern void calc_curve_bezier_endp(REAL xend, REAL yend, REAL xadj, REAL yadj,
 extern void free_installed_fonts(void);

extern BOOL lengthen_path(GpPath *path, INT len);
+extern BOOL lengthen_path_strict(GpPath *path, INT len);

extern DWORD write_region_data(const GpRegion *region, void *data);
 extern DWORD write_path_data(GpPath *path, void *data);
diff --git a/dlls/gdiplus/graphicspath.c b/dlls/gdiplus/graphicspath.c
index 24c2888cfe8..7b85355773a 100644
--- a/dlls/gdiplus/graphicspath.c
+++ b/dlls/gdiplus/graphicspath.c
@@ -1184,14 +1184,7 @@ GpStatus WINGDIPAPI GdipClonePath(GpPath* path, GpPath **clone)

**clone = *path;

-    (*clone)->pathdata.Points = malloc(path->datalen * sizeof(PointF));
-    (*clone)->pathdata.Types = malloc(path->datalen);
-    if(!(*clone)->pathdata.Points || !(*clone)->pathdata.Types){
-        free((*clone)->pathdata.Points);
-        free((*clone)->pathdata.Types);
-        free(*clone);
-        return OutOfMemory;
-    }
+    if (!lengthen_path_strict(*clone, path->datalen)) return OutOfMemory;

memcpy((*clone)->pathdata.Points, path->pathdata.Points,
            path->datalen * sizeof(PointF));
@@ -1284,15 +1277,7 @@ GpStatus WINGDIPAPI GdipCreatePath2(GDIPCONST GpPointF* points,
         }
     }

-    (*path)->pathdata.Points = malloc(count * sizeof(PointF));
-    (*path)->pathdata.Types = malloc(count);
-
-    if(!(*path)->pathdata.Points || !(*path)->pathdata.Types){
-        free((*path)->pathdata.Points);
-        free((*path)->pathdata.Types);
-        free(*path);
-        return OutOfMemory;
-    }
+    if (!lengthen_path_strict(*path, count)) return OutOfMemory;

memcpy((*path)->pathdata.Points, points, count * sizeof(PointF));
     memcpy((*path)->pathdata.Types, types, count);
diff --git a/dlls/gdiplus/metafile.c b/dlls/gdiplus/metafile.c
index 5c50d0d1d37..30e69ee4668 100644
--- a/dlls/gdiplus/metafile.c
+++ b/dlls/gdiplus/metafile.c
@@ -2039,16 +2039,9 @@ static GpStatus metafile_deserialize_path(const BYTE *record_data, UINT data_siz
     if (status != Ok)
         return status;

-    (*path)->pathdata.Count = data->PathPointCount;
-    (*path)->pathdata.Points = malloc(data->PathPointCount * sizeof(*(*path)->pathdata.Points));
-    (*path)->pathdata.Types = malloc(data->PathPointCount * sizeof(*(*path)->pathdata.Types));
-    (*path)->datalen = (*path)->pathdata.Count;
+    if (!lengthen_path_strict(*path, data->PathPointCount)) return OutOfMemory;

-    if (!(*path)->pathdata.Points || !(*path)->pathdata.Types)
-    {
-        GdipDeletePath(*path);
-        return OutOfMemory;
-    }
+    (*path)->datalen = (*path)->pathdata.Count;

if (data->PathPointFlags & 0x4000) /* C */
     {

[PATCH 2/3] dlls/gdiplus: Empty path before lengthening it in GdipFlattenPath.

[David Kahurani]

From: David Kahurani

Empty the path, albeit temporarily before trying to lengthen
the path because the data contained in it is at this point,
unless an error happens, invalid. This avoids a situation where
lengthening code, assuming the data in the path is valid
proceeds to lengthen the path further.

Signed-off-by: David Kahurani 
---
 dlls/gdiplus/graphicspath.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/dlls/gdiplus/graphicspath.c b/dlls/gdiplus/graphicspath.c
index 7b85355773a..ea9f1ed9aed 100644
--- a/dlls/gdiplus/graphicspath.c
+++ b/dlls/gdiplus/graphicspath.c
@@ -1333,7 +1333,7 @@ GpStatus WINGDIPAPI GdipFlattenPath(GpPath *path, GpMatrix* matrix, REAL flatnes
 {
     path_list_node_t *list, *node;
     GpPointF pt;
-    INT i = 1;
+    INT i = 1, old_count;
     INT startidx = 0;
     GpStatus stat;

@@ -1345,6 +1345,7 @@ GpStatus WINGDIPAPI GdipFlattenPath(GpPath *path, GpMatrix* matrix, REAL flatnes
     if(path->pathdata.Count == 0)
         return Ok;

+    old_count = path->pathdata.Count;
     stat = GdipTransformPath(path, matrix);
     if(stat != Ok)
         return stat;
@@ -1407,6 +1408,7 @@ GpStatus WINGDIPAPI GdipFlattenPath(GpPath *path, GpMatrix* matrix, REAL flatnes

/* store path data back */
     i = path_list_count(list);
+    path->pathdata.Count = 0;
     if(!lengthen_path(path, i))
         goto memout;
     path->pathdata.Count = i;
@@ -1422,6 +1424,7 @@ GpStatus WINGDIPAPI GdipFlattenPath(GpPath *path, GpMatrix* matrix, REAL flatnes
     return Ok;

memout:
+    path->pathdata.Count = old_count;
     free_path_list(list);
     return OutOfMemory;
 }

[PATCH 3/3] dlls/gdiplus: Empty path before lengthening it in GdipWidenPath.

[David Kahurani]

From: David Kahurani

Empty the path, albeit temporarily before trying to lengthen
the path because the data contained in it is at this point,
unless an error happens, invalid. This avoids a situation where
lengthening code, assuming the data in the path is valid
proceeds to lengthen the path further.

Signed-off-by: David Kahurani 
---
 dlls/gdiplus/graphicspath.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/dlls/gdiplus/graphicspath.c b/dlls/gdiplus/graphicspath.c
index ea9f1ed9aed..b40e84c07f3 100644
--- a/dlls/gdiplus/graphicspath.c
+++ b/dlls/gdiplus/graphicspath.c
@@ -2465,7 +2465,7 @@ GpStatus WINGDIPAPI GdipWidenPath(GpPath *path, GpPen *pen, GpMatrix *matrix,
     GpPath *flat_path=NULL;
     GpStatus status;
     path_list_node_t *points=NULL, *last_point=NULL;
-    int i, subpath_start=0, new_length;
+    int i, subpath_start=0, new_length, old_count;

TRACE("(%p,%p,%s,%0.2f)\n", path, pen, debugstr_matrix(matrix), flatness);

@@ -2475,6 +2475,8 @@ GpStatus WINGDIPAPI GdipWidenPath(GpPath *path, GpPen *pen, GpMatrix *matrix,
     if (path->pathdata.Count <= 1)
         return OutOfMemory;

+    old_count = path->pathdata.Count;
+
     status = GdipClonePath(path, &flat_path);

if (status == Ok)
@@ -2550,8 +2552,12 @@ GpStatus WINGDIPAPI GdipWidenPath(GpPath *path, GpPen *pen, GpMatrix *matrix,

new_length = path_list_count(points)-1;

+        path->pathdata.Count = 0;
         if (!lengthen_path(path, new_length))
+        {
+            path->pathdata.Count = old_count;
             status = OutOfMemory;
+        }
     }

if (status == Ok)

[PATCH 3/3] dlls/gdiplus: Empty path before lengthening it in GdipWidenPath.

[Marvin]

Hi,

It looks like your patch introduced the new failures shown below.
Please investigate and fix them before resubmitting your patch.
If they are not new, fixing them anyway would help a lot. Otherwise
please ask for the known failures list to be updated.

The tests also ran into some preexisting test failures. If you know how
to fix them that would be helpful. See the TestBot job for the details:

The full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=141650

Your paranoid android.

=== debian11 (32 bit report) ===

gdiplus:
metafile.c:3986: Test failed: Expected -00065536, got -16777216
metafile.c:3990: Test failed: Expected -00065536, got -16777216

=== debian11b (64 bit WoW report) ===

gdiplus:
metafile.c:3986: Test failed: Expected -00065536, got -16777216
metafile.c:3990: Test failed: Expected -00065536, got -16777216

[Esme Povirk (＠madewokherd)]

Esme Povirk (@madewokherd) commented about dlls/gdiplus/gdiplus.c:
>  return TRUE;
>
> }
>
> +BOOL lengthen_path_strict(GpPath *path, INT len)
> +{
> - /* Strictly allocate the requested memory size */
> - /* Used only on fresh paths */
> - path->pathdata.Points = calloc(len, sizeof(PointF));
> - path->pathdata.Types = calloc(1, len);
> -
> - if (!path->pathdata.Points || !path->pathdata.Types)
> - {
> -
>    free(path->pathdata.Points);
> -
>    free(path->pathdata.Types);
> -
>    free(path);

If this can free the path as a side-effect, it should also create it.

[Esme Povirk (＠madewokherd)]

Esme Povirk (@madewokherd) commented about dlls/gdiplus/graphicspath.c:
>  /* store path data back */
>  i = path_list_count(list);
> - path->pathdata.Count = 0;

I think it would make more sense to pass in the number of additional points needed. Either that or construct an entirely new GpPathData and then free/replace the old one in case of error.

[Esme Povirk (＠madewokherd)]

We should probably just have a function that replaces a path's contents with the contents of a point list.