On Wed, 2007-11-21 at 12:00 -0500, Stephen Smalley wrote: > On Wed, 2007-11-21 at 11:44 -0500, Joshua Brindle wrote: > > Stephen Smalley wrote: > > > On Tue, 2007-11-20 at 17:09 -0500, Joshua Brindle wrote: > > > > > >> So, I got a report here that python scripts that call > > >> semanage_select_store will get a double free error when calling > > >> semanage_handle_destroy because the swig wrapper free's the buffer > > >> passed to semanage_select_store. I haven't seen this with semanage but > > >> the bug does appear legitimate to me, we should have never been assuming > > >> control of the buffer passed to semanage_select_store (especially > > >> without documenting it). > > >> > > >> This is fun because semanage_select_store was originally a void but > > >> clearly this needs to strdup() instead of just setting the pointer. This > > >> patch can be applied trunk as it changes the API but the bug would still > > >> exist in stable (and the person who reported this will unfortunately > > >> have to patch it in their release of the stable repo). > > >> > > > > > > Wait - we aren't changing API/ABI even in trunk at present, and to do > > > so, we'd either need to change .so version (incompatible change) or use > > > the per-symbol versioning technique to keep the old interface alive for > > > existing binaries while defaulting to the new interface for newer code. > > > > > > I also have to ask - if the problem lies in the swig wrapper, why not > > > fix it there? > > > > > > > > > > Because I'm scared of the swig wrapper :) > > Well, we shouldn't be more afraid of changing the python bindings that > we are of changing the library API and ABI. Ok, so maybe there is no way to fix this in the wrapper, as the wrapper has to create a temporary C string from the python string and then free it when done with the call. Which would mean that we need to strdup all strings passed into the library. > > But seriously we always strdup (afaik) strings passed in to libsemanage > > so this is an inconsistency and should at least be documented. > > The key create functions don't appear to do so, e.g. > semanage_seuser_key_create. Don't know how widespread the issue is. > Why does semanage appear to work ok? Hmm...well semanage -S/--store doesn't appear to work at all right now, not sure why. As far as keys go, they are different in that we don't free the names in the key_free functions, so no double free there, but it still appears that they would be prematurely freed by the wrapper while still in use by the library. Not sure then why we don't have lots of bug reports on semanage. > > I suppose we can just start queuing up any api/abi changes for some > > future so version, this certainly doesn't warrant bumping the version. > > It could be done via per-symbol versioning rather than a .so version > change. > > > OTOH I understand that we need a stable ABI in trunk but honestly I > > don't know how this change would cause problems (how many external users > > are there, how often is this error condition going to occur (esp > > compared to how often the error condition it is fixing can occur), etc). > > If we do need to make the change, then I think we want to systematically > fix all such occurrences at once, and do it cleanly via per-symbol > versioning or .so version bump. Which suggests doing it in policyrep > first. But if we can make a change to the swig wrapper instead to fix > the bug, then that would be nice for stable and for a short term fix for > trunk. Let's do it this way then: - change the functions to strdup() internally always. If the function presently returns void and the strdup fails, then abort(). If the function presently returns int, then return an error of course. We can apply that change on the current trunk and stable. - plan to change those void functions to return int, and queue that up in policyrep. But that can come later. Then we can turn those aborts into returns. -- Stephen Smalley National Security Agency -- This message was distributed to subscribers of the selinux mailing list. If you no longer wish to subscribe, send mail to majordomo@xxxxxxxxxxxxx with the words "unsubscribe selinux" without quotes as the message.
