Skip to content
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

Fixing several issues reported by coverity scan #5375

Closed
wants to merge 1 commit into from
Closed

Fixing several issues reported by coverity scan #5375

wants to merge 1 commit into from

Conversation

zdohnal
Copy link
Contributor

@zdohnal zdohnal commented Aug 15, 2018

Here is cleaner commit...

@zdohnal
Copy link
Contributor Author

zdohnal commented Aug 15, 2018

All issues in report which sent you were labeled as important, so there are other issues, but coverity doesn't count them as important.

Copy link
Collaborator

@michaelrsweet michaelrsweet left a comment

Choose a reason for hiding this comment

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

I need to look more closely at one of the changes, and will want to use a different change for cups-driverd.

@@ -3624,4 +3624,7 @@ update_reasons(ipp_attribute_t *attr, /* I - printer-state-reasons or NULL */
fprintf(stderr, "%s\n", add);
else if (rem[0])
fprintf(stderr, "%s\n", rem);

if (new_reasons != NULL)
cupsArrayDelete(new_reasons);
Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, we should just initialize new_reasons to NULL up top and then call cupsArrayDelete unconditionally (it handles NULLs just fine.)

@@ -3916,6 +3916,7 @@ http_create(
{
_cupsSetError(IPP_STATUS_ERROR_INTERNAL, strerror(errno), 0);
httpAddrFreeList(addrlist);
httpAddrFreeList(myaddrlist);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I need to look at this more closely; my gut says we should just be freeing myaddlist and not addrlist (since, if supplied, we copy addrlist to myaddrlist...)

@@ -1076,7 +1076,7 @@ cupsRasterWriteHeader(

void *dst = fh.cupsReal; /* Bypass bogus compiler warning */
void *src = r->header.cupsReal;
memcpy(dst, src, sizeof(fh.cupsReal) + sizeof(fh.cupsString));
memcpy(dst, src, sizeof(fh.cupsReal));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is wrong. We need to copy everything in cupsReal and cupsString - cupsString follows cupsReal in the structure so this is valid.

*/
free(device_id_re);
free(make_and_model_re);

Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't care about this because main() calls list_ppds() and then returns, freeing all memory used by the program. Perhaps I should have it not return and instead exit().

@michaelrsweet
Copy link
Collaborator

I pushed changes for the full scan report to branch-2.2 and master:

[master 3416fe9] Fix memory leaks found by Coverity (Issue #5375)

[branch-2.2 1a3ff20] Fix memory leaks found by Coverity (Issue #5375)

@zdohnal
Copy link
Contributor Author

zdohnal commented Aug 27, 2018

I apologize for late reply, I was on vacation last week - I'll run it through coverity scan and I'll check with you about results.

@zdohnal
Copy link
Contributor Author

zdohnal commented Sep 21, 2018

I checked the commit with coverity - the results are okay, no new important reports. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants