using sparse to catch refcount leaks
Hi,
I was thinking the same mechanism for lock checks (lock() has matching unlock()) could be used for detecting refcount leaks (get() has matching put()).
This could be used to catch bugs like that:
Cheers,
Signature · 4 lines
Aurélien Aptel / SUSE Labs Samba Team GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
Quoted 9 lines · Aurélien Aptel
Hi,
I was thinking the same mechanism for lock checks (
lock()has matchingunlock()) could be used for detecting refcount leaks (get()has matchingput()).This could be used to catch bugs like that:
Absolutely.
Sparse's 'context' can be reused for refcounting (conceptually, it's quite similar to locking). You can even simply reuse the existing macros __acquires(), ...
That's said, I suppose that in the present case the 'get' is done by cifs_sb_tlink(). This corresponds to a 'conditional context' (see __cond_lock()) which somehow annoying to use. I've some patches improving things here but they need some more work.
Cheers,
-- Luc
Quoted 9 lines · Aurélien Aptel
Hi,
I was thinking the same mechanism for lock checks (
lock()has matchingunlock()) could be used for detecting refcount leaks (get()has matchingput()).This could be used to catch bugs like that:
You might be better off copying the Smatch's locking test instead of Sparse's. Smatch does cross function flow analysis which can help.
In this CIFS case, the leak is on the success path (as well as the failure path) so my theory would be that it would be caught in testing and will only cause false positives for static analysis. I can't see any automated way to know which success paths should take a reference and which should not.
regards,
dan carpenter
Quoted 5 lines · Dan Carpenter
In this CIFS case, the leak is on the success path (as well as the failure path) so my theory would be that it would be caught in testing and will only cause false positives for static analysis. I can't see any automated way to know which success paths should take a reference and which should not.
Ah, I see. I guess with extra annotations and more sophisticated analysis we could have something but it seems out of scope.
Anyway, thanks for looking into it.
Cheers,
Signature · 4 lines
Aurélien Aptel / SUSE Labs Samba Team GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)