GNOME Bugzilla – Bug 105291
AccessibleTextChangedEvent_getChangeString semms to return const gchar*
Last modified: 2004-12-22 21:47:04 UTC
AccessibleTextChangedEvent_getChangeString seems to return a const value. According with API in at-spi/cspi/spi.h, returned value could be released when no longer needed, but when user do that a crash happening. Other negativ effect is that the pointer is valid only if the event is valid, which, in my opinion is not OK.
Created attachment 14315 [details] [review] Proposed patch
Does the propsoed patch fix the problem?
thanks Padraig, I think this is the right thing to do; please commit! [though it's normal to have to dup a CORBA string if you want to persist it, we don't really want to expose the CORBA-ness of this value so making it an alloc'd string makes the most sense. We couldn't really have made the return const AFAICS]
Patch committed to CVS HEAD.
Padraig, in your patch you used g_strdup. That means that g_free must be called to free allocated memory. To be consistent with other at-spi functions which returns gchar* SPI_freeString must be called on returned value. If this functions is called application which do that crashes.
Certainly using CORBA_string_dup in place of g_strdup (which is indeed wrong, sorry about that) would make this problem go away, but I am a little surprised that it's necessary, since the string is already coming from a demarshalled CORBA value. I think my occasional fuzzyheadedness about CORBA lifecycle stuff has bitten us here; it seems that the CORBA demashalled value get freed by the CORBA mechanism when we exit the method call. So if the client is expected to free the result, we need to CORBA_string_dup the value. But I am still unclear on exactly why this is true, since similar duplication is not required for similar calls such as AccessibleText_getText. I think it has to do with the fact the the event itself (containing the string stuff as internal data) is getting freed, which results in a free of the internal data; therefore we need to duplicate it before doing a client-side explicit 'CORBA_free' on the string. [SPI_freeString () is really just a wrapper around CORBA_free]
Created attachment 14793 [details] [review] New patch
Is the new patch the correct fix?
I think so, but then again I got it wrong last time :-)
Created attachment 14795 [details] [review] Updated patch
I propose to apply the updated patch as it confirms that crash does not occur.
thanks Padraig, go ahead and apply!
Applied patch to CVS HEAD.