Skip to content

Fix errors pointed out by static analyzer #943

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.

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

Merged
merged 2 commits into from
Sep 4, 2023

Conversation

Medvecrab
Copy link
Contributor

A static analyzer found some errors -

  1. Deref of Null - admin.c

Return value of a function 'pktbuf_dynamic' is dereferenced at admin.c:245 without checking for NULL, but it is usually checked for this function (25/26).

  1. Memory Leak - proto.c

Dynamic memory, referenced by 'ibuf', is allocated at proto.c:474 by calling function 'malloc' and lost at proto.c:488.

  1. Memory Leak - scram.c

Dynamic memory, referenced by 'server_signature_base64', is allocated at scram.c:932 by calling function 'malloc' and lost at scram.c:938.

  1. No VA End - proto.c : 686;

Function 'va_end' was not called before proto.c:686 inside function 'scan_text_result'. (Function 'va_start' was called at proto.c:624.)

  1. No VA End - proto.c : 693

Function 'va_end' was not called before proto.c:693 inside function 'scan_text_result'. (Function 'va_start' was called at proto.c:624.)

So here's proposed fixes

src/proto.c Outdated
@@ -683,14 +683,17 @@ int scan_text_result(struct MBuf *pkt, const char *tupdesc, ...)
int newlen;
if (strncmp(val, "\\x", 2) != 0) {
log_warning("invalid bytea value");
va_end(ap);
Copy link
Member

@JelteF JelteF Sep 3, 2023

Choose a reason for hiding this comment

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

To avoid mistakes like this in the future, let's use the goto failed pattern for this function.

Copy link
Member

@JelteF JelteF left a comment

Choose a reason for hiding this comment

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

Thank you, looks good overall. But left one small suggestion for an improvement.

@Medvecrab
Copy link
Contributor Author

Added suggested change, thanks for review!

@JelteF
Copy link
Member

JelteF commented Sep 4, 2023

FYI the formatting check is failing, make format-c should fix that.

@Medvecrab
Copy link
Contributor Author

Force-pushed a formatting fix

@JelteF JelteF merged commit bb018a6 into pgbouncer:master Sep 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants