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.
