Hi Stephen, Thank you for pointing out errors in the patch. Checkpolicy is a tool for build time. I agree the memory leak fix has very little run-time value. That said, I still hope the fix will make the tool a little bit better than what it was. For module_compiler.c I decided to leave it unchanged. I will include the Klocwork report along with the patch when I submit to selinux mailing list, so the tool maintainer has the info to decide what to do about it. I also removed the unnecessary initializations that you pointed out. As for the test, I'm still using the build / diff approach mentioned in my previous email. All the memory leak happen at error handling which are not exercised during a successful build. So I hope the review will catch the incorrect ones.
Thanks again, Alice ________________________________________ From: Stephen Smalley [[email protected]] Sent: Wednesday, January 02, 2013 8:14 AM To: Alice Chu Cc: [email protected]; William Roberts Subject: Re: Fixing checkpolicy issues found by Klocwork On 12/21/2012 08:27 PM, Alice Chu wrote: > Hello, > > Attached you will find the Klocwork report on external/checkpolicy and my fix > in a patch. The change is done on master branch. > Please review and let me know any additional correction I should make. > > This is how I tested the fix: > (1) Prior to any change, did a full build and saved the following files > aside: > out/target/product/generic/root/file_contexts > out/target/product/generic/root/property_contexts > out/target/product/generic/root/seapp_contexts > out/target/product/generic/root/sepolicy > > out/target/product/generic/obj/ETC/sepolicy_intermediates/policy.conf > (2) Did a clean full build after the change and compared the above files > with the newly created ones. > The test result: the corresponding files are identical. > > Thank you very much for your feedback. > > Regards, > Alice Chu Technically patches for libsepol or checkpolicy should go to the selinux mailing list, not seandroid-list, as they are merely copies of released versions of the upstream SELinux libsepol and checkpolicy. And inlined patches are better for review as we can then easily quote portions of the patch with our comments interspersed. But with regard to your patch, I believe it is incorrect. Note that the original code does not call class_datum_destroy() on the datum if the datum was successfully inserted into the classes table by require_symbol(). Also note that the original code frees the allocated datum if require_symbol() returned 1 and then resets datum to the result of searching the classes table, after which we do not want to ever free that datum. Whereas your patch can yield situations where we will free a datum that is still referenced by the classes table. Several of your other changes are likewise either wrong or unnecessary (e.g. initializing to NULL when the variable is immediately set to a value returned by malloc before any further use). And your test case is insufficient to test the condition that is allegedly being fixed, as you aren't actually exercising those error paths. I should also note that the potential value of these changes is relatively low as checkpolicy will exit on any errors during policy parsing and thus any transient memory leaks on the error paths will ultimately be recovered when it exits. -- This message was distributed to subscribers of the seandroid-list mailing list. If you no longer wish to subscribe, send mail to [email protected] with the words "unsubscribe seandroid-list" without quotes as the message.
