Bug 1119290 - cups-browsed leaks memory
Summary: cups-browsed leaks memory
Keywords:
Status: CLOSED UPSTREAM
Alias: None
Product: Fedora
Classification: Fedora
Component: cups-filters
Version: 20
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Tim Waugh
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-07-14 12:46 UTC by Jan Vesely
Modified: 2014-10-08 12:23 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-10-08 12:23:15 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)
valgrind trace (4.67 KB, text/plain)
2014-07-14 12:46 UTC, Jan Vesely
no flags Details
valgrind trace (4.62 KB, text/plain)
2014-07-15 19:22 UTC, Jan Vesely
no flags Details
cups-browsed-memory-leak.patch (834 bytes, patch)
2014-07-16 07:41 UTC, Tim Waugh
no flags Details | Diff
another fix (632 bytes, patch)
2014-10-06 16:07 UTC, Jan Vesely
no flags Details | Diff
memory-leak-fix.bundle (1.93 KB, text/plain)
2014-10-08 11:17 UTC, Tim Waugh
no flags Details

Description Jan Vesely 2014-07-14 12:46:19 UTC
Created attachment 917784 [details]
valgrind trace

Description of problem:
cups-browsed leaks a lot of memory (similar to #1027317

Version-Release number of selected component (if applicable):
cups-filters-1.0.53-5.fc20.x86_64

How reproducible:
always

Steps to Reproduce:
1. run cups-browsed service
2. watch the memory use grow
3.

Actual results:


Expected results:


Additional info:
I've attached valgrind trace of cups-browse running overnight

Comment 1 Jiri Popelka 2014-07-14 13:30:31 UTC
(In reply to Jan Vesely from comment #0)
> I've attached valgrind trace of cups-browse running overnight

Thanks Jan,

there seems to be missing debuginfo for cups-filters package - see the '??? (in /usr/sbin/cups-browsed)' in the trace.
I don't see any leaks in my instance of cups-browsed so I need you to install it with 'debuginfo-install cups-filters' and try to run it again. Thank you.

Comment 2 Jan Vesely 2014-07-15 19:22:01 UTC
Created attachment 918234 [details]
valgrind trace

this trace has all the symbol information, hope it helps

Comment 3 Tim Waugh 2014-07-16 07:41:44 UTC
Created attachment 918340 [details]
cups-browsed-memory-leak.patch

Thanks.

Fix sent upstream.

Comment 4 Jan Vesely 2014-10-06 16:07:51 UTC
Created attachment 944317 [details]
another fix

(In reply to Tim Waugh from comment #3)
> Created attachment 918340 [details]
> cups-browsed-memory-leak.patch
> 
> Thanks.
> 
> Fix sent upstream.

Hi,

is there a way to track this? I could not find the patch on cups-filters ML, or fedora pkg-git (maybe i need to look harder).

One question about the patch. Wouldn't ippDelete on line 749 lead to a possible use-after-free?
if 'printer-name' is not in the first sequence of IPP_TAG_PRINTER attributes (and the outer most for loop does another iteration)
I don't know whether such response format is possible.

Comment 5 Tim Waugh 2014-10-08 11:02:58 UTC
Oh, yes, it would have lead to a use-after-free, thanks for spotting it.

I think this patch should be enough to fix 1.0.60:

--- cups-filters-1.0.60/utils/cups-browsed.c~	2014-10-01 19:25:42.000000000 +0100
+++ cups-filters-1.0.60/utils/cups-browsed.c	2014-10-08 11:39:24.754004293 +0100
@@ -761,6 +761,7 @@ gboolean handle_cups_queues(gpointer unu
 	  httpClose(http);
 	  /* Schedule the removal of the queue for later */
 	  p->timeout = current_time + TIMEOUT_RETRY;
+	  ippDelete(response);
 	  break;
 	}
 	if (response)

Basically we just need to make sure to ippDelete(response) once the printer-name attribute has been evaluated. What do you think?

Comment 6 Tim Waugh 2014-10-08 11:14:39 UTC
CCing Till for review.

Comment 7 Tim Waugh 2014-10-08 11:17:55 UTC
Created attachment 944944 [details]
memory-leak-fix.bundle

Here's that change as a bzr bundle.

Comment 8 Till Kamppeter 2014-10-08 12:00:03 UTC
Tim, thank you for the patch. It is absolutely correct.

I have applied it upstream on the BZR repository (rev. 7297) now, so the change will be part of the 1.0.61 release.

Comment 9 Tim Waugh 2014-10-08 12:23:15 UTC
Thanks.


Note You need to log in before you can comment on or make changes to this bug.