Attempt to fix CID 530895 #949

cmb69 · 2025-01-18T15:30:29Z

If gaussian_coeffs() was called with a non-positve radius, bad things could happen, but we don't that; we add an assert() as safety net, and to enlighten static analyzers.

Frankly, I'm not sure what exactly is CoverityScan complaining about when they compain that sum might be zero, but this pre-condition assert() seems to be a good idea anyway.

We should probably also make sure that we don't call gaussian_coeffs() with a radius > INT_MAX / 2 - 1 (or something like that; possibly using overflow2()).

If `gaussian_coeffs()` was called with a non-positve `radius`, bad things could happen, but we don't that; we add an `assert()` as safety net, and to enlighten static analyzers.

vapier · 2025-01-23T14:28:13Z

src/gd_filter.c

@@ -842,6 +842,8 @@ gaussian_coeffs(int radius, double sigmaArg) {
    double sum = 0;
    int x, n, count;

+    assert(radius >= 1);


we should never use assert() in library code that users can get to. would be nice if we could add a CI check to ban it in all library code. tests & examples are fine to use assert/etc... of course.

we should return errors back up, and if we think it's helpful (e.g. for bad API calls), log an error via gd_error.

This is in a static function which is only called by the gdImageCopyGaussianBlurred() API which alread has:

libgd/src/gd_filter.c

Lines 1009 to 1011 in 0b8d708

if (radius < 1) {

return NULL;

}/* if */

This assert() is only a sanity check that we use the static function properly, and also a hint for static analyzers (and manual code review).

the nuance when it's "ok" to use assert and when people are just being lazy is very hard to communicate. people see it being used in some places and think it's ok in general.

since it's a static function, the compiler should inline it. and the compiler should be able to see the higher level has the same check and elide one of them. a quick check over here suggests it does, so the generated code is the same.

For me, assert() is a development tool. Release builds should always define NDEBUG, so asserts are a no-op (in my opinion, it would be better to have that the other way round: need to define DEBUG to enable asserts). We doing this:

libgd/src/gd_filter.c

Lines 23 to 28 in 0b8d708

#undef NDEBUG

/* Comment out this line to enable asserts.

* TODO: This logic really belongs in cmake and configure.

*/

#define NDEBUG 1

#include <assert.h>

(and yes, I agree that should be moved to configure)

enabling extra debug in a library should not cause it to abort and kill a process. assert usage in programs is different from libraries. we should have no asserts in the gd library regardless of its compilation mode. the existing asserts are "ok" in the sense that they always force NDEBUG which means the calls are empty. interesting that coverity accepts that behavior ... I can see both sides there.

Attempt to fix CID 530895

b2bc5a5

If `gaussian_coeffs()` was called with a non-positve `radius`, bad things could happen, but we don't that; we add an `assert()` as safety net, and to enlighten static analyzers.

vapier requested changes Jan 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attempt to fix CID 530895 #949

Attempt to fix CID 530895 #949

	#undef NDEBUG
	/* Comment out this line to enable asserts.
	* TODO: This logic really belongs in cmake and configure.
	*/
	#define NDEBUG 1
	#include <assert.h>

Attempt to fix CID 530895 #949

Are you sure you want to change the base?

Attempt to fix CID 530895 #949

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment