Attempt to fix CID 530895#949
Conversation
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.
| double sum = 0; | ||
| int x, n, count; | ||
|
|
||
| assert(radius >= 1); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This is in a static function which is only called by the gdImageCopyGaussianBlurred() API which alread has:
Lines 1009 to 1011 in 0b8d708
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
Lines 23 to 28 in 0b8d708
(and yes, I agree that should be moved to configure)
There was a problem hiding this comment.
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.
If
gaussian_coeffs()was called with a non-positveradius, bad things could happen, but we don't that; we add anassert()as safety net, and to enlighten static analyzers.Frankly, I'm not sure what exactly is CoverityScan complaining about when they compain that
summight be zero, but this pre-conditionassert()seems to be a good idea anyway.We should probably also make sure that we don't call
gaussian_coeffs()with aradius > INT_MAX / 2 - 1(or something like that; possibly usingoverflow2()).