Re: [POLICYREP] [RFC/PATCH 2/3] policy package implementation

[Karl MacMillan <kmacmillan@mentalrootkit.com>]

On Tue, 2007-07-17 at 12:40 -0400, Joshua Brindle wrote:
> Karl MacMillan wrote:
[...]
> >> +			// Currently we use the name of the file to decide what kind of file it is
> >> +			// I am fairly uncomfortable with this but it allows us to use xar to create
> >> +			// policy packages instead of having our own write_package function that 
> >> +			// assigns attributes to files instead of using names. 
> >>     
> >
> > I don't like this either - what is the alternative?
> >
> >   
> 
> Ok, xar has an XML ToC for the file. It also provides an API to set 
> arbitrary attributes for each file, so I can do something like 
> selinuxfiletype = netfilter_contexts. This means that we would not be 
> able to use the command line xar to create packages (I was trying to 
> make this as simple as possible). If we move to using attributes we'd 
> have to have a custom packager again (ala semodule_package).
> 

It would be simpler though, correct? And what about an unpackager?

> A custom packager has advantages, we can basically ensure that the 
> package is sane (doesn't have directory trees or multiple 
> <something>.netfilter_contexts files, etc. We can also add new 
> properties such as the policy name to the xar XML subdocument.
> 

That sounds better than relying on file extension to me.

> >> +			ret = xar_prop_get(f, "name", &filename);
> >> +			if (ret) {
> >> +				xar_close(x);
> >> +				throw "Error getting name property of file";
> >>     
> >
> > Please define an exception.
> >
> >   
> Why?
> 

Because none of the standard exception types seemed to fit and string
exceptions are not a good idea.

> >> +			}
> >> +			if (strstr(filename, ".mod")) {
> >> +				// found module, do stuff
> >>     
> >
> > Err - "do stuff"?
> >
> >   
> There is no module_read function right now so thats just a placeholder.
> 

There is if the module is source :)

> >> +			} else if (strstr(filename, ".file_contexts")) {
> >> +				if (!impl->file_contexts.empty()) {
> >> +					// Found more than one file containing in .file_contexts,
> >> +					// don't know how to handle that so bail for now
> >> +					xar_close(x);
> >> +					throw "Multiple file_contexts files in policy package";
> >> +				}
> >> +				impl->file_contexts = std::string(fbuf);
> >> +			} else if (strstr(filename, ".seusers")) {
> >> +				if (!impl->seusers.empty()) {
> >> +					// Found more than one file containing in .seusers,
> >> +					// don't know how to handle that so bail for now
> >> +					xar_close(x);
> >> +					throw "Multiple seusers files in policy package";
> >> +				}
> >> +				impl->seusers = std::string(fbuf);
> >>     
> >
> > As we discussed privately - these copies are not great. Perhaps just use
> > raw char*.
> >
> >   
> 
> Ok, I don't know if this code is ever going to do anything with those 
> and was trying to keep it in a format that would be more accessible if 
> we needed to. I can change to char * though and see if anything develops 
> that makes std::string make more sense.
> 
> >> +			} else if (strstr(filename, ".user_extra")) {
> >> +				if (!impl->user_extra.empty()) {
> >> +					// Found more than one file containing in .user_extra,
> >> +					// don't know how to handle that so bail for now
> >> +					xar_close(x);
> >> +					throw "Multiple user_extra files in policy package";
> >> +				}
> >> +				impl->user_extra = std::string(fbuf);
> >> +			} else if (strstr(filename, ".netfilter_contexts")) {
> >> +				if (!impl->netfilter_contexts.empty()) {
> >> +					// Found more than one file containing in .netfilter_contexts,
> >> +					// don't know how to handle that so bail for now
> >> +					xar_close(x);
> >> +					throw "Multiple netfilter_contexts files in policy package";
> >> +				}
> >> +				impl->netfilter_contexts = std::string(fbuf);
> >> +			} else {
> >> +				// unrecognized file, just skip it
> >> +				continue;
> >> +			}
> >> +
> >> +		}
> >> +
> >> +		xar_close(x);
> >> +
> >> +		return 0;
> >> +	}
> >> +
> >> +	PolicyPackage::PolicyPackage() {
> >> +		init();
> >> +	}
> >> +
> >> +	PolicyPackage::PolicyPackage(std::string filename) {
> >> +		init();
> >> +		read_package(filename);
> >> +	}
> >> +
> >> +	PolicyPackage::~PolicyPackage() { delete impl; }
> >> +
> >> +	const Module PolicyPackage::get_policy_module() const {
> >> +		return impl->policy_module;
> >> +	}
> >>     
> >
> > This should definitely not be by value. It should also likely not be
> > const - this is how the user will retrieve the module after reading to
> > use, correct? Then it shouldn't be const (they might want to much with
> > it), should be a reference, and shouldn't be const for the class (as
> > allowing users to change the module changes the object).
> >
> >   
> Fair enough. I was basically thinking users would read a module and make 
> their own copies (eg., copying the data into a merged copy or something) 
> but that isn't really how policyrep works so I'll change all these to be 
> non-const.
> 

One other note - if the caller is expected to take ownership for the
lifetime of the object (which seems possible), then it should return a
pointer or a shared_ptr.

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.