8000 Attempt to fix CID 530895 by cmb69 · Pull Request #949 · libgd/libgd · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Attempt to fix CID 530895 #949

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Attempt to fix CID 530895 #949

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/gd_filter.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -842,6 +842,8 @@ gaussian_coeffs(int radius, double sigmaArg) {
double sum = 0;
int x, n, count;

assert(radius >= 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

54A4

count = 2*radius + 1;

result = gdMalloc(sizeof(double) * count);
Expand Down
Loading
0