[cairo] [PATCH cairo v2 0/8] Coverity fixes

If the bitmap's min is non-zero, _bitmap_next_id() could break out of
its loop early, before initializing the prev variable. prev would then
be dereferenced without a null ptr check. This condition should never
occur in practice, so add an assert() to assure it doesn't.

Same issue is present in trace.c.

Coverity IDs: #1159557, #1159558

Signed-off-by: Bryce Harrington <***@bryceharrington.org>
---
src/cairo-script-surface.c | 1 +
util/cairo-trace/trace.c | 2 ++
2 files changed, 3 insertions(+)

diff --git a/src/cairo-script-surface.c b/src/cairo-script-surface.c
index e715cae..7db7dc5 100644
--- a/src/cairo-script-surface.c
+++ b/src/cairo-script-surface.c
@@ -262,6 +262,7 @@ _bitmap_next_id (struct _bitmap *b,
prev = &b->next;
b = b->next;
} while (b != NULL);
+ assert (prev != NULL);

bb = _cairo_malloc (sizeof (struct _bitmap));
if (unlikely (bb == NULL))
diff --git a/util/cairo-trace/trace.c b/util/cairo-trace/trace.c
index 3c05613..87b2df4 100644
--- a/util/cairo-trace/trace.c
+++ b/util/cairo-trace/trace.c
@@ -299,8 +299,10 @@ _type_next_token (Type *t)
prev = &b->next;
b = b->next;
}
+ assert (prev != NULL);

bb = malloc (sizeof (struct _bitmap));
+
*prev = bb;
bb->next = b;
bb->min = min;

--
2.7.4
--
cairo mailing list
***@cairographics.org
https://lists.cairographics

Bryce Harrington

2018-06-13 00:35:32 UTC

_cairo_malloc_ab() can return NULL under some circumstances, and all
other callers of this routine in the Cairo codebase check its return, so
do so here as well.

Coverity ID: #1159556

Reviewed-by: Uli Schlachter <***@znc.in>
Signed-off-by: Bryce Harrington <***@bryceharrington.org>
---
src/cairo-bentley-ottmann.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/cairo-bentley-ottmann.c b/src/cairo-bentley-ottmann.c
index 91e41f9..afe3a63 100644
--- a/src/cairo-bentley-ottmann.c
+++ b/src/cairo-bentley-ottmann.c
@@ -1484,10 +1484,13 @@ _cairo_bentley_ottmann_tessellate_polygon (cairo_traps_t *traps,
ymin = _cairo_fixed_integer_floor (polygon->limit.p1.y);
ymax = _cairo_fixed_integer_ceil (polygon->limit.p2.y) - ymin;

- if (ymax > 64)
+ if (ymax > 64) {
event_y = _cairo_malloc_ab(sizeof (cairo_bo_event_t*), ymax);
- else
+ if (unlikely (event_y == NULL))
+ return _cairo_error (CAIRO_STATUS_NO_MEMORY);
+ } else {
event_y = stack_event_y;
+ }
memset (event_y, 0, ymax * sizeof(cairo_bo_event_t *));
}

--
2.7.4
--
cairo mailing list
***@cairographics.org

Bryce Harrington

2018-06-13 00:35:33 UTC

Note this changes the semantics of the value of extra_out such that it
is set to NULL instead of left undefined in case an error is returned.

Coverity ID: 220086

Signed-off-by: Bryce Harrington <***@bryceharrington.org>
---
src/cairo-surface-snapshot.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/cairo-surface-snapshot.c b/src/cairo-surface-snapshot.c
index c8f3078..a8b8c0e 100644
--- a/src/cairo-surface-snapshot.c
+++ b/src/cairo-surface-snapshot.c
@@ -100,14 +100,17 @@ _cairo_surface_snapshot_acquire_source_image (void *abstract_
cairo_status_t status;

extra = _cairo_malloc (sizeof (*extra));
- if (unlikely (extra == NULL))
+ if (unlikely (extra == NULL)) {
+ *extra_out = NULL;
return _cairo_error (CAIRO_STATUS_NO_MEMORY);
+ }

extra->target = _cairo_surface_snapshot_get_target (&surface->base);
status = _cairo_surface_acquire_source_image (extra->target, image_out, &extra->extra);
if (unlikely (status)) {
cairo_surface_destroy (extra->target);
free (extra);
+ extra = NULL;
}

*extra_out = extra;

--
2.7.4
--
cairo mailing list
***@cairographics.org
https://lists.c

Bryce Harrington

2018-06-13 00:35:34 UTC

If the call to _cairo_malloc_ab_plus_c() fails, it returns an error
without first freeing event_y.

Coverity ID: #1160682

Signed-off-by: Bryce Harrington <***@bryceharrington.org>
---
src/cairo-bentley-ottmann.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/cairo-bentley-ottmann.c b/src/cairo-bentley-ottmann.c
index afe3a63..2011e73 100644
--- a/src/cairo-bentley-ottmann.c
+++ b/src/cairo-bentley-ottmann.c
@@ -1501,8 +1501,11 @@ _cairo_bentley_ottmann_tessellate_polygon (cairo_traps_t *traps,
sizeof (cairo_bo_start_event_t) +
sizeof (cairo_bo_event_t *),
sizeof (cairo_bo_event_t *));
- if (unlikely (events == NULL))
+ if (unlikely (events == NULL)) {
+ if (event_y != stack_event_y)
+ free (event_y);
return _cairo_error (CAIRO_STATUS_NO_MEMORY);
+ }

event_ptrs = (cairo_bo_event_t **) (events + num_events);
}

--
2.7.4
--
cairo mailing list
***@cairographics.org
https://lists

Bryce Harrington

2018-06-13 00:35:35 UTC

Patch 37a22669 improved performance by using bounding box extents.
However, the code appears to be incorrect. If extents is non-NULL it
copies its contents to group->extents, otherwise it sets group->extents
to sensible defaults, but then goes ahead and tries to copy the
undefined contents. This second copy is unnecessary if extents is
non-NULL and will cause a crash if it is NULL.

Drop the extra copy, guessing it's just a typo.

Coverity ID: #1159559

Signed-off-by: Bryce Harrington <***@bryceharrington.org>
---
src/cairo-pdf-surface.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/src/cairo-pdf-surface.c b/src/cairo-pdf-surface.c
index 7186fc0..ab67813 100644
--- a/src/cairo-pdf-surface.c
+++ b/src/cairo-pdf-surface.c
@@ -1291,7 +1291,6 @@ _cairo_pdf_surface_create_smask_group (cairo_pdf_surface_t *surface,
group->extents.width = surface->width;
group->extents.height = surface->height;
}
- group->extents = *extents;

return group;
}

--
2.7.4
--
cairo mailing list
***@cairographics.org
https://lists.cairographics.org/mailman/lis

Bryce Harrington

2018-06-13 00:35:36 UTC

num_glyphs and num_clusters are explicitly checked to be non-NULL at the
beginning of this routine, and by this point in the code both have been
deref'd multiple times, so checking them for NULL here again is
superfluous.

It looks like the intent here is to verify the glyphs and clusters
arrays are non-NULL unless their counts are zero, so change the tests
accordingly.

Coverity ID: #983386

Signed-off-by: Bryce Harrington <***@bryceharrington.org>
---
src/cairo-scaled-font.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/cairo-scaled-font.c b/src/cairo-scaled-font.c
index f7a36c1..8dff57d 100644
--- a/src/cairo-scaled-font.c
+++ b/src/cairo-scaled-font.c
@@ -2052,7 +2052,7 @@ cairo_scaled_font_text_to_glyphs (cairo_scaled_font_t *scaled_font,
status = _cairo_error (CAIRO_STATUS_NEGATIVE_COUNT);
goto DONE;
}
- if (num_glyphs && *glyphs == NULL) {
+ if (*num_glyphs != 0 && *glyphs == NULL) {
status = _cairo_error (CAIRO_STATUS_NULL_POINTER);
goto DONE;
}
@@ -2062,7 +2062,7 @@ cairo_scaled_font_text_to_glyphs (cairo_scaled_font_t *scaled_font,
status = _cairo_error (CAIRO_STATUS_NEGATIVE_COUNT);
goto DONE;
}
- if (num_clusters && *clusters == NULL) {
+ if (*num_clusters != 0 && *clusters == NULL) {
status = _cairo_error (CAIRO_STATUS_NULL_POINTER);
goto DONE;
}

--
2.7.4
--
cairo mailing list
***@cairographics.org
https://lists.cairographics.org

Bryce Harrington

2018-06-13 00:35:37 UTC

subrs was already tested for NULL prior to this, and will never be NULL
at this point. Meanwhile, find_token()'s return is unchecked (it can
return NULL and is checked in all other calls). Quite clearly, this is
a copy-paste error from the prior find_token call, and the intent was to
check array_start not subrs.

Coverity ID: #1160662

Signed-off-by: Bryce Harrington <***@bryceharrington.org>
---
src/cairo-type1-subset.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/cairo-type1-subset.c b/src/cairo-type1-subset.c
index 89cb96f..5f07456 100644
--- a/src/cairo-type1-subset.c
+++ b/src/cairo-type1-subset.c
@@ -1331,7 +1331,7 @@ cairo_type1_font_subset_write_private_dict (cairo_type1_font_subset_t *font,

/* look for "dup" which marks the beginning of the first subr */
array_start = find_token (subr_count_end, font->cleartext_end, "dup");
- if (subrs == NULL)
+ if (array_start == NULL)
return CAIRO_INT_STATUS_UNSUPPORTED;

/* Read in the subroutines */

--
2.7.4
--
cairo mailing list
***@cairographics.org
https:

Bryce Harrington

2018-06-13 00:35:38 UTC

The code is checking a variable is non-NULL after it's already been
dereferenced in an assert.

I'm not certain whether the assert should be conditionalized to only be
tested when right != NULL (which would allow edges_end() to still be
invoked), or if the function should assert right as non-NULL always.

Coverity ID: #1160730

Signed-off-by: Bryce Harrington <***@bryceharrington.org>
---
src/cairo-polygon-intersect.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/cairo-polygon-intersect.c b/src/cairo-polygon-intersect.c
index 9c1777f..001e55e 100644
--- a/src/cairo-polygon-intersect.c
+++ b/src/cairo-polygon-intersect.c
@@ -1107,13 +1107,14 @@ edges_start_or_continue (cairo_bo_edge_t *left,
int top,
cairo_polygon_t *polygon)
{
+ assert (right != NULL);
assert (right->deferred.other == NULL);

if (left->deferred.other == right)
return;

if (left->deferred.other != NULL) {
- if (right != NULL && edges_colinear (left->deferred.other, right)) {
+ if (edges_colinear (left->deferred.other, right)) {
cairo_bo_edge_t *old = left->deferred.other;

/* continuation on right, extend right to cover both */
@@ -1131,7 +1132,7 @@ edges_start_or_continue (cairo_bo_edge_t *left,
edges_end (left, top, polygon);
}

- if (right != NULL && ! edges_colinear (left, right)) {
+ if (! edges_colinear (left, right)) {
left->deferred.top = top;
left->deferred.other = right;
}

--
2.7.4
--
cairo mailing list
***@cairographics.org
http

Uli Schlachter

2018-06-13 17:54:45 UTC

Patches 1, 3, 4, 5, 7, and 8 (well, and 2) are

Reviewed-By: Uli Schlachter <***@znc.in>

I ran into a mental timeout when staring at Patch 6, so I skipped that
one (for now?).

I have one remaining question: The test suite is happy with this and
doesn't hit the new assertions, right?

Cheers,
Uli

Post by Bryce Harrington
This patchset attempts to fix a handful of Cairo issues found by
Coverity. A lot of the Coverity results seem to be false positives, but
these look legit.
The first three patches were presented in the v1 of this series and have
been updated based on Uli's review feedback. The remainder are new in
this series. Patch #4 was suggested by Uli. Patches 5, 6, and 7 I'm
fairly confident were just cut-and-paste typos, but a sanity check would
be appreciated on them.
Patch 8 deserves closer scrutiny. The coverity issue is legit as the
code is indeed doing something ugly, but I'm not sure my fix is correct.
script-surface: Check for invalid ids (CID #1159557, 1159558)
bo: Check null return from _cairo_malloc_ab() (CID #1159556)
snapshot: Don't use extra after it's been freed (CID #220086)
bo: Free event_y in case of error to prevent memory leak (CID
##1160682)
pdf: Fix potential null ptr deref when creating smask groups (CID
#1159559)
scaled-font: Fix glyph and cluster count checks (CID #983386)
type1-subset: Fix incorrect null ptr check from find_token() (CID
#1160662)
polygon-intersection: Clarify ptr checks for right edges (CID
#1160730)
src/cairo-bentley-ottmann.c | 12 +++++++++---
src/cairo-pdf-surface.c | 1 -
src/cairo-polygon-intersect.c | 5 +++--
src/cairo-scaled-font.c | 4 ++--
src/cairo-script-surface.c | 1 +
src/cairo-surface-snapshot.c | 5 ++++-
src/cairo-type1-subset.c | 2 +-
util/cairo-trace/trace.c | 2 ++
8 files changed, 22 insertions(+), 10 deletions(-)

--
- He wants the impossible...!
- That's the short definition of 'captain'.
--
cairo mailing list
***@cairographics.org
https://lists.cairograp

Bryce Harrington

2018-06-13 23:11:11 UTC

Post by Uli Schlachter
Patches 1, 3, 4, 5, 7, and 8 (well, and 2) are
I ran into a mental timeout when staring at Patch 6, so I skipped that
one (for now?).

Thanks for reviewing the 7, I'll push those now. Patch 6 I think is
good but will hold off for you to look at it more.

Post by Uli Schlachter
I have one remaining question: The test suite is happy with this and
doesn't hit the new assertions, right?

It seems to be, yes. It looks like the patches only affect the image,
pdf, and script backends so limiting the test run to just those:

== Ubuntu 16.04.4 results ==
$ make test TARGETS=image,pdf,script FORMAT=rgba

trunk: 347 Passed, 210 Failed [0 crashed, 16 expected], 21 Skipped
patched: 347 Passed, 210 Failed [0 crashed, 16 expected], 21 Skipped

== Ubuntu 18.04 results (image backend only) ==
$ make test TARGETS=image FORMAT=rgba

trunk: 495 Passed, 55 Failed [0 crashed, 14 expected], 28 Skipped
patched: 495 Passed, 55 Failed [0 crashed, 14 expected], 28 Skipped

Hopefully you also see no failure deltas; let me know if otherwise.

Patch 6 also gives the same testsuite results, no added failures.

Bryce

--
- He wants the impossible...!
- That's the short definition of 'captain'.

--
cairo mailing list
***@cairographics.org
https://lists.cairographics.org/mailman/li

Uli Schlachter

2018-06-15 16:55:15 UTC

Post by Uli Schlachter
Patches 1, 3, 4, 5, 7, and 8 (well, and 2) are
I ran into a mental timeout when staring at Patch 6, so I skipped that
one (for now?).

Thanks for reviewing the 7, I'll push those now. Patch 6 I think is
good but will hold off for you to look at it more.

Well, okay. The patch looks fine and I am not even sure what my problem
was last time I looked at it.

Post by Uli Schlachter
I have one remaining question: The test suite is happy with this and
doesn't hit the new assertions, right?

It seems to be, yes. It looks like the patches only affect the image,

[...]

Post by Bryce Harrington
Hopefully you also see no failure deltas; let me know if otherwise.

I didn't try. :-)
Just wanted to make sure someone did try.

Uli

Post by Bryce Harrington
Bryce

--
- He wants the impossible...!
- That's the short definition of 'captain'.

--
Sent from my Game Boy.
--
cairo mailing list
***@cairographics.org
https://lists.cairographics.org/mailman/listinfo/ca

Bryce Harrington

2018-06-16 05:18:28 UTC

Post by Uli Schlachter
Patches 1, 3, 4, 5, 7, and 8 (well, and 2) are
I ran into a mental timeout when staring at Patch 6, so I skipped that
one (for now?).

Thanks for reviewing the 7, I'll push those now. Patch 6 I think is
good but will hold off for you to look at it more.

Well, okay. The patch looks fine and I am not even sure what my problem
was last time I looked at it.

Great, thanks, I've landed it.

Bryce

Post by Uli Schlachter
I have one remaining question: The test suite is happy with this and
doesn't hit the new assertions, right?

It seems to be, yes. It looks like the patches only affect the image,

[...]

Post by Bryce Harrington
Hopefully you also see no failure deltas; let me know if otherwise.

I didn't try. :-)
Just wanted to make sure someone did try.
Uli

Post by Bryce Harrington
Bryce