Re: [g-a-devel]patch to make test-simple work again ...

[Michael Meeks <michael ximian com>]

Hi Bill,

On Mon, 2002-01-07 at 20:04, Bill Haneman wrote:
> BH: I'm used to doing strncpy, etc. because of stack overflow

	strncpy copies to a static buffer, g_strdup is like strdup which has ~0
security issues.

> exploits... it doesn't look to me as if g_strdup guards against failure
> of g_new0 to allocate a big enough buffer.

	Running out of memory on a system with virtual memory results in a
fatal trap in glib; there is no security issue with g_strdup.

>  Mebbe I am wrong here, and just momentarily paranoid...

	Yep :-) I updated that to the faster copy - ideally we'd manipulate
this string on the stack though.

> BH: reversibility would be intellectually satisfying but I don't know
> that it's necessary here.  The first part of the string is used for
> other stuff but is always the same by the time the string parse func is
> called.
> BH: I'll explain this func in more detail some time...

	I would like to understand what / why it is doing so much dynamic
allocation, and umpteen strlen's on strings we've already measured etc.
- we do a lot of work here; and it's per event.

> BH: Personally I think we should wait with this.  The struct is not in
> fact arbitrary ;-), and there are more important fixes to make at the moment;
> optimizations here can surely wait, though I certainly don't have
> anything against fast code.  

	Whatever.

> 	It is important to do list tracking operations before poking at the
> contents of the list in a way that can cause re-enterancy; ie. we need
> to do:
> 
> 	... = g_list_delete_link (..., obj's element);
> 	bonobo_object_release_unref (obj, NULL);
> 
> 	Not:
> 
> !	bonobo_object_release_unref (obj, NULL);
> !	... = g_list_delete_link (..., obj's element);
> 
> BH: How can deleting a link cause the bonobo object to be touched? 

	It can't.

> BH: Seems to me that this can only be a problem in a multi-threaded environment.

	Not at all; when you call bonobo_object_release_unref a CORBA method is
invoked, at which point _any_ method can re-enter. Consider one that
modifies the list pointer; or perhaps does another 'delete' on the
'element'.

	In which case you have a floating, invalid pointer lying around to that
element, that you then do an operation on - very bad news.

> -static long _get_unique_id();
> 
> BH: I still maintain that it's better to make
> BH: the unique id assignment programmatic, in case someday we have
> multiple
> BH: copies of this data segment (OK, edge case that is in the future,
> but still...)

	I've never seen such a thing; for the app to be threaded would be
amazing, to fork and split into two registryd's seems unthinkable.
Nevertheless - I've moved the static int increment out into it's own
method again.

> +#ifdef USE_A_HASH_IN_FUTURE
...
> +#endif
> 
> ??? what's with the guards here?  Looks like these methods is still
> called...  couldn't we just make this a TODO, I don't think the
> guards help readability at all. ;-)

	The methods are not called; hence the guards kill warnings, whilst
leaving code that might be useful if we use a hash in future - although
using a hash would give us even more acute ( and unfixable )
re-enterancy problems.

> BH: Otherwise the patch looks good, though there was a lot of
> interleaving
> in the diff making it really hard to read...
> BH: Again, thanks for putting this one together...

	No problem - I'll just add the split of the common re-enterant list
walkers to the mix that you thought was ok on IRC and commit.

	Regards,

		Michael.

-- 
 mmeeks gnu org  <><, Pseudo Engineer, itinerant idiot