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.
> 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?
> 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.
>
> >> ---
> >>
> >> libsemanage/include/semanage/handle.h | 3 ++-
> >> libsemanage/src/handle.c | 13 ++++++++++---
> >> policycoreutils/semanage/seobject.py | 4 +++-
> >> policycoreutils/semodule/semodule.c | 3 ++-
> >> 4 files changed, 17 insertions(+), 6 deletions(-)
> >>
> >> ---
> >>
> >> Index: libsemanage/include/semanage/handle.h
> >> ===================================================================
> >> --- libsemanage/include/semanage/handle.h (revision 2677)
> >> +++ libsemanage/include/semanage/handle.h (working copy)
> >> @@ -48,8 +48,9 @@
> >> /* This function allows you to specify the store to connect to.
> >> * It must be called after semanage_handle_create but before
> >> * semanage_connect. The argument should be the full path to the store.
> >> + * path is duplicated and should be freed by the caller.
> >> */
> >> -void semanage_select_store(semanage_handle_t * handle, char *path,
> >> +int semanage_select_store(semanage_handle_t * handle, char *path,
> >> enum semanage_connect_type storetype);
> >>
> >> /* Just reload the policy */
> >> Index: libsemanage/src/handle.c
> >> ===================================================================
> >> --- libsemanage/src/handle.c (revision 2677)
> >> +++ libsemanage/src/handle.c (working copy)
> >> @@ -28,6 +28,7 @@
> >> #include <stdlib.h>
> >> #include <stdio.h>
> >> #include <sys/time.h>
> >> +#include <string.h>
> >>
> >> #include "direct_api.h"
> >> #include "handle.h"
> >> @@ -123,7 +124,7 @@
> >> return sh->is_connected;
> >> }
> >>
> >> -void semanage_select_store(semanage_handle_t * sh, char *storename,
> >> +int semanage_select_store(semanage_handle_t * sh, char *storename,
> >> enum semanage_connect_type storetype)
> >> {
> >>
> >> @@ -131,10 +132,16 @@
> >>
> >> /* This just sets the storename to what the user requests, no
> >> verification of existance will be done until connect */
> >> - sh->conf->store_path = storename;
> >> +
> >> + /* sh->conf->store_path could already exist if it was read from the
> >> + * config file, free first */
> >> + free(sh->conf->store_path);
> >> + sh->conf->store_path = strdup(storename);
> >> + if (!sh->conf->store_path)
> >> + return -1;
> >> sh->conf->store_type = storetype;
> >>
> >> - return;
> >> + return 0;
> >> }
> >>
> >> int semanage_is_managed(semanage_handle_t * sh)
> >> Index: policycoreutils/semanage/seobject.py
> >> ===================================================================
> >> --- policycoreutils/semanage/seobject.py (revision 2677)
> >> +++ policycoreutils/semanage/seobject.py (working copy)
> >> @@ -219,7 +219,9 @@
> >> raise ValueError(_("Could not create semanage handle"))
> >>
> >> if store != "":
> >> - semanage_select_store(self.sh, store, SEMANAGE_CON_DIRECT);
> >> + rc = semanage_select_store(self.sh, store, SEMANAGE_CON_DIRECT);
> >> + if rc:
> >> + raise ValueError(_("Could not set store."))
> >>
> >> self.semanaged = semanage_is_managed(self.sh)
> >>
> >> Index: policycoreutils/semodule/semodule.c
> >> ===================================================================
> >> --- policycoreutils/semodule/semodule.c (revision 2677)
> >> +++ policycoreutils/semodule/semodule.c (working copy)
> >> @@ -293,7 +293,8 @@
> >> * this will always set a direct connection now, an additional
> >> * option will need to be used later to specify a policy server
> >> * location */
> >> - semanage_select_store(sh, store, SEMANAGE_CON_DIRECT);
> >> + if (semanage_select_store(sh, store, SEMANAGE_CON_DIRECT))
> >> + goto cleanup;
> >> }
> >>
> >> /* if installing base module create store if necessary, for bootstrapping */
> >>
> >>
> >>
>
--
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.
