Created attachment 155951 [details] Fix (incomplete) Found by clang static analyzer: http://scan.freebsd.org/scan-build/WORLD/2015-04-20-amd64/report-851c0f.html#EndPath And also Coverity (CID 1007770) I am not using this so hopefully someone else can review/test.
(In reply to Pedro F. Giffuni from comment #0) Wrong link, see: http://scan.freebsd.org/scan-build/WORLD/2015-04-20-amd64/report-16a93b.html#EndPath
Looks like it needs a little more than that, as with just your patch it could free more than once.
Created attachment 156014 [details] Fix avoiding double free Thanks! Quick fix.
Why not just move the free out the loop?
Comment on attachment 156014 [details] Fix avoiding double free (In reply to Steven Hartland from comment #4) I think assigning the NULLs is actually unnecessary: all the occasions where scan_info is free'd go to a break. The first patch seems fine. Moving the free() out of the loop would be a possibility but seems better to release the memory as soon as it is used, and I suspect that's what the author had in mind. OTOH, but this is just me, I prefer to avoid modifying existing code since this is not something I can test.
Your missing he fact when they break out of the loop above. The leak will only occur when no break in above for loop happens, however in the common case where it does, your free will currently be a double free.
Comment on attachment 156014 [details] Fix avoiding double free (In reply to Steven Hartland from comment #6) Bah.. yes you are right.
A commit references this bug: Author: pfg Date: Wed Apr 29 15:46:58 UTC 2015 New revision: 282227 URL: https://svnweb.freebsd.org/changeset/base/282227 Log: Fix memory leak in scsi_scan_bus() CID: 1007770 PR: 199671 Changes: head/sys/cam/scsi/scsi_xpt.c
It looks pretty safe. Thank you Steven for the review!
Patch reverted in r282239. False positive: Fatal trap 12: page fault while in kernel mode cpuid = 0; apic id = 00 fault virtual address = 0x21000002d8 fault code = supervisor read data, page not present instruction pointer = 0x20:0xffffffff802fd074 stack pointer = 0x28:0xfffffe100678f960 frame pointer = 0x28:0xfffffe100678f9e0 code segment = base rx0, limit 0xfffff, type 0x1b = DPL 0, pres 1, long 1, def32 0, gran 1 processor eflags = interrupt enabled, resume, IOPL = 0 current process = 2 (doneq0) [ thread pid 2 tid 100025 ] Stopped at scsi_scan_bus+0x54: movq 0x58(%rax),%rdi db> bt Tracing pid 2 tid 100025 td 0xfffff8000d3ac940 scsi_scan_bus() at scsi_scan_bus+0x54/frame 0xfffffe100678f9e0 xpt_done_process() at xpt_done_process+0x521/frame 0xfffffe100678fa20 xpt_done_td() at xpt_done_td+0xf6/frame 0xfffffe100678fa70 fork_exit() at fork_exit+0x71/frame 0xfffffe100678fab0 fork_trampoline() at fork_trampoline+0xe/frame 0xfffffe100678fab0 --- trap 0, rip = 0, rsp = 0xfffffe100678fb70, rbp = 0 --- db>
yer scan_info is still referenced in work_ccb, I should have spotted that apologies.
(In reply to Steven Hartland from comment #11) Not really your fault: I have been trusting too much the static analyzers lately, and they clearly may miss things, *especially* in the kernel.
Perhaps a comment should be added to the source code indicating why it the analysis is a false positive.
(In reply to cartwright from comment #13) Comments for things that don't exist and don't help development in any way are generally considered noise. There is the commit history and this bug report which serve as reference. The bug is in the static analyzer so I annotated it in Coverity. Clang doesn't currently have tags for this type of errors. Of course if many people are looking at the issue and repeatedly try to fix what is not broken it may be better to add a comment but we currently don't have many people checking those reports (most FreeBSD developers are indeed perfect ;) ).
The clang static analyzer wasn't wrong. It's possible to leak the scan_info object if the conditions for the 'for' loop prevent the loop from being entered. It's not a normal occurrence in real life, but it's not impossible either. What needs to happen is that the loop increment a reference counter on scan_info, and if at the exit of the loop the refcount is 0, then free scan_info. It's not a bug in clang, and it's probably not in Coverity either, but I haven't looked at the output from that. Please don't file it as a bug with those projects.
(In reply to Scott Long from comment #15) OK, I un-flagged it as "false positive" in Coverity. There was never a bug report as those projects are pretty much used to having both false-positives and true-positives that are tagged incorrectly as false-positives.