policyrep questions

policyrep questions

[Karl MacMillan]

I have a few questions as I continue to do the policyrep patches.

1) The free functions for the objects may need to return an error (which
I hate, but there is no good way to avoid this). This will break
compatibility with the existing string based sepol objects (like
sepol_bool). Any problems breaking this compatibility?

2) The general form of the free functions will be:

int sepol_object_free(struct sepol_handle *h, struct sepol_object *o);

Is the handle necessary? The general form is to pass a handle wherever
an error is possible, but it seems like overkill in this case.

3) The existing objects copy the passed in strings where I was hoping to
avoid the copy. Any opinions either way? I'm currently leaning towards
copying because that kind of change will create hard to track down bugs
for no good reason.

4) We assume NULL-terminated strings all over the place - should we be
providing apis with length? Alternatively, should we provide a better
string object (since we are currently re-inventing the wheel, why not?).
We could pull in something like James Antills Vstr library -
http://www.and.org/vstr/ (read and be amazed at the diligence of James).
It just seems crazy that our apis are not the safest.

Karl




--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: policyrep questions

[Karl MacMillan]

On Tue, 2007-05-08 at 18:29 -0400, Karl MacMillan wrote:
> 
> 3) The existing objects copy the passed in strings where I was hoping to
> avoid the copy. Any opinions either way? I'm currently leaning towards
> copying because that kind of change will create hard to track down bugs
> for no good reason.
> 

To make things more confusing - some of the existing code copies strings
while others do not. For example sepol_bool_set_name copies the string
while sepol_bool_key_create does not. I assume that the thought was that
the keys were temporary and likely to be populated from other sources
(existing bools, user input, etc.).

However - I think that this inconsistency is just confusing. I'm going
to change all of the functions to copy the strings. Changing in this
direction means the worst case is that a few strings will be leaked.

Karl


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: policyrep questions

[Stephen Smalley]

On Wed, 2007-05-09 at 10:13 -0400, Karl MacMillan wrote:
> On Tue, 2007-05-08 at 18:29 -0400, Karl MacMillan wrote:
> > 
> > 3) The existing objects copy the passed in strings where I was hoping to
> > avoid the copy. Any opinions either way? I'm currently leaning towards
> > copying because that kind of change will create hard to track down bugs
> > for no good reason.
> > 
> 
> To make things more confusing - some of the existing code copies strings
> while others do not. For example sepol_bool_set_name copies the string
> while sepol_bool_key_create does not. I assume that the thought was that
> the keys were temporary and likely to be populated from other sources
> (existing bools, user input, etc.).
> 
> However - I think that this inconsistency is just confusing. I'm going
> to change all of the functions to copy the strings. Changing in this
> direction means the worst case is that a few strings will be leaked.

Yes, I think that the failure to copy the keys was also a complaint of
the setools folks.

-- 
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@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: policyrep questions

[Stephen Smalley]

On Tue, 2007-05-08 at 18:29 -0400, Karl MacMillan wrote:
> I have a few questions as I continue to do the policyrep patches.
> 
> 1) The free functions for the objects may need to return an error (which
> I hate, but there is no good way to avoid this). This will break
> compatibility with the existing string based sepol objects (like
> sepol_bool). Any problems breaking this compatibility?

Not a problem (.so version has already changed in -policyrep and we have
already made other incompatible changes).

OTOH, what could/would a caller do if a free function failed?  At that
point, it will leak memory unless it aborts altogether, right?  So
possibly we gain nothing from returning an error status to the caller?
What do other libraries do under similar conditions?  Or do they avoid
it through different data structure and API design?

> 2) The general form of the free functions will be:
> 
> int sepol_object_free(struct sepol_handle *h, struct sepol_object *o);
> 
> Is the handle necessary? The general form is to pass a handle wherever
> an error is possible, but it seems like overkill in this case.

Only if you plan to generate an error message from within the function.

> 3) The existing objects copy the passed in strings where I was hoping to
> avoid the copy. Any opinions either way? I'm currently leaning towards
> copying because that kind of change will create hard to track down bugs
> for no good reason.

Yes, I think copying the data and managing its lifecycle within the
library is safer.

> 4) We assume NULL-terminated strings all over the place - should we be
> providing apis with length? Alternatively, should we provide a better
> string object (since we are currently re-inventing the wheel, why not?).
> We could pull in something like James Antills Vstr library -
> http://www.and.org/vstr/ (read and be amazed at the diligence of James).
> It just seems crazy that our apis are not the safest.

I'm not clear that tracking length separately is advantageous for these
APIs.  What specific advantages would accrue to libsepol from using
vstr?  What is the cost (incl. dependencies)?

-- 
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@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: policyrep questions

[Karl MacMillan]

On Wed, 2007-05-09 at 11:29 -0400, Stephen Smalley wrote:
> On Tue, 2007-05-08 at 18:29 -0400, Karl MacMillan wrote:
> > I have a few questions as I continue to do the policyrep patches.
> > 
> > 1) The free functions for the objects may need to return an error (which
> > I hate, but there is no good way to avoid this). This will break
> > compatibility with the existing string based sepol objects (like
> > sepol_bool). Any problems breaking this compatibility?
> 
> Not a problem (.so version has already changed in -policyrep and we have
> already made other incompatible changes).
> 
> OTOH, what could/would a caller do if a free function failed?

It's not clear - it will be _very_ hard to guarantee that the data
structures are in some sort of semi-valid state so that the free
function could be called again.

>   At that
> point, it will leak memory unless it aborts altogether, right?  So
> possibly we gain nothing from returning an error status to the caller?

Basically - the worst case is a leak. And bear in mind, this is a
failure case caused by the inability to allocate a couple of bytes.
Chances are we are about to hit many other unrecoverable errors.

To make this worse - there are several cases where I ignore failures in
free functions. In error paths that free an object I usually want to
return the original error and not the error on free - so I ignore the
return from the free functions.

> What do other libraries do under similar conditions?  Or do they avoid
> it through different data structure and API design?
> 

No idea. It is hard to fully encapsulate all structs and avoid this
issue I think. Even if I made it so that the library could internally
put all of the structs on the stack users of the library would be faced
with exactly the same set of issues.

I vote to suppress all of these free errors (which don't occur often -
most of these functions have error returns just for the consistency that
is needed for the object system).

> 
> > 4) We assume NULL-terminated strings all over the place - should we be
> > providing apis with length? Alternatively, should we provide a better
> > string object (since we are currently re-inventing the wheel, why not?).
> > We could pull in something like James Antills Vstr library -
> > http://www.and.org/vstr/ (read and be amazed at the diligence of James).
> > It just seems crazy that our apis are not the safest.
> 
> I'm not clear that tracking length separately is advantageous for these
> APIs.

It might be more efficient in many cases. Obviously we can't protect
against malicious callers, but forcing people to pass length makes them
aware of the issues I think.

>   What specific advantages would accrue to libsepol from using
> vstr?

We are going to start wanting to build up some complex strings as part
of the policyrep work. That kind of concatenation is painful with the C
string APIS (and inefficient). Vstr is also designed to be easy /
efficient to use to push strings across the network.

>   What is the cost (incl. dependencies)?
> 

I don't think there are any deps. It's not in fedora yet, but it will be
soon.

Karl


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: policyrep questions

[James Antill]

[-- Attachment #1: Type: text/plain, Size: 1424 bytes --]

On Wed, 2007-05-09 at 11:29 -0400, Stephen Smalley wrote:

> > 4) We assume NULL-terminated strings all over the place - should we be
> > providing apis with length? Alternatively, should we provide a better
> > string object (since we are currently re-inventing the wheel, why not?).
> > We could pull in something like James Antills Vstr library -
> > http://www.and.org/vstr/ (read and be amazed at the diligence of James).
> > It just seems crazy that our apis are not the safest.
> 
> I'm not clear that tracking length separately is advantageous for these
> APIs.

 I assume you mean "tracking length separately is not advantageous"?

>   What specific advantages would accrue to libsepol from using
> vstr?  What is the cost (incl. dependencies)?

 I'm not sure you need all the power of Vstr, while it certainly
provides a lot of convenience functions that you could use the main
design focus was on IO strings that travel across the network ... not on
lots of small strings in hashtables etc.
 The Vstr lib. itself has no deps.

 However I am convinced that having just char* is not a suitable string
ADT, tracking the length/size separately never works out well from a
bugs POV.
 Also sepol seems to do a _lot_ of comparisons, being able to implement
"eq" as:

 (s1->len == s1->len && !memcmp(s1->ptr, s2->ptr, s1->len)

...is a major win.

-- 
James Antill <jantill@redhat.com>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]