On Tue, 2007-04-24 at 19:12 -0400, James Antill wrote:
> On Mon, 2007-04-23 at 17:34 -0400, jbrindle@tresys.com wrote:
> > This is the majority of the patches from the policy server release a few months ago. This implements object serialization to send objects (eg., booleans, file contexts, etc) across the line to the policy server. It also implements the line protocol to connect to the policy server and the backend for libsemanage so that semodule, semanage, etc will talk to a policy server instead of doing local operations.
> > 
> > The object serialization will also be necessary for the policy representation branch as we will use this infrastructure to serialize the policy tree for module reading and writing.
> > 
> > The only part left for the policy server is the hooks that were implemented in the expander. As the policy representation work is going on and should remove the expander entirely these patches will have to wait until we have enough of the new representation work to implement them there.
> > 
> > This is obviously meant only for trunk and the policyrep branch.
> 
>  Karl asked me to have a look at this, and I haven't looked at all of
> it ... but what I have seen worries me a bit.


 Here's the first parts:

[patch 01]
 
int sepol_serialize(sepol_handle_t * handle,
		    const void *datum,
		    size_t datum_length,
		    unsigned int datum_type, char **data, uint64_t * size)
		    
 This is better fixed by Karl's suggestion of not having a single function, but having datum_length which is ignored for SEPOL_SERIAL_INT32_T etc. is confusing.

 The serialization for SEPOL_SERIAL_STRING:
  Why are you using snprintf()?
  You are moving datum_length into status (size_t into int), which is just asking for pain.

int sepol_unserialize(sepol_handle_t * handle,
		char **data, uint64_t * size,
		void **datum,
		size_t ** datum_length,
		unsigned int datum_type)

SEPOL_SERIAL_STRING_ARRAY:
                               /* Datum. */
				*datum =
					calloc(sizeof(char *),
							sizeof(char *) * (**datum_length));

 I'm pretty sure you want calloc(sizeof(char *), **datum_length);

[patch 10]

 
 int dbase_serialize(struct semanage_handle *handle,
		    dbase_config_t * dconfig,
		    char **data, uint64_t * data_length)

 size_t can still be 32bits, so storing uint64_t's in it might be bad.
 I'm not sure I understand what the completed_count is doing, are you leaking anything if the calloc() fails? I think you want to free upto count all the time. 

[patch 12]
 semanage_serialize is an exact copy of sepol_serialize, AFAICS, so see above.
 
[patch 16]

 I assume semanage_fcontext_get_con(fcontext) doesn't allocate anything, still it'd be nice to not call it twice and just reuse con.
 
 I worry about having semanage_fcontext_unserialize() leave fcontext allocated on failure. A bunch of code does this with arrays ... but even so.
 
[patch 21]
 
 Dito. user_extra is left allocated on the failure path, in semanage_user_extra_unserialize.
 Dito. user is left allocated on the failure path, in semanage_user_unserialize.
 
[patch 22]
  
 for (i = 0; i < *modules_size; i++) {
		/* Module name. */
		status = semanage_unserialize(handle, data, size, (void **)&(*modules)[i].name, &temp_size, SEMANAGE_SERIAL_STRING);
		if (status != STATUS_SUCCESS)
			goto cleanup;

		/* Module version. */
		status = semanage_unserialize(handle, data, size, (void **)&(*modules)[i].version, &temp_size, SEMANAGE_SERIAL_STRING);
		if (status != STATUS_SUCCESS)
		        goto cleanup;
}

 temp_size is allocated on _each call_ to unserialize. 
 
[patch 23]
 
 Why are you calling serialize twice, INT32_T is a fixed size, no?

-- 
James Antill <jantill@redhat.com>