[pulseaudio-discuss] ABI breakage in introspect (85e7fbc196f4424f68e530c2e3a01d9b941f293e)
Colin Guthrie
gmane at colin.guthr.ie
Thu Jan 9 02:07:38 PST 2014
Hi,
In order to get Bluetooth support (at least A2DP) I've been tracking the
5.0 release for Mageia (it's generally an unfortunate situation but such
is life).
Sadly there appears to be some ABI breakage introduced related to card
profiles.
This is purportedly fixed by the below commit but I think there are
still issues.
I've had crashes reported in both KDE's kcm_phonon (the bits which which
I wrote) and pavucontrol directly, both related to updating card info.
It seems invalid string information is passed in to string compare
functions and they crash.
https://bugs.mageia.org/show_bug.cgi?id=12155
I can't reproduce the problem on my machine but as soon as I can find a
system which fails, I will write a fix if someone doesn't beat me to it.
These crashes are reported with
2747c961015ba00ec9a1cad8a8a95b4a34db9ee0
I'll update to master but I don't see any changes to introspect.c that
would suggest any fix in master since then.
Col
'Twas brillig, and Tanu Kaskinen at 05/11/13 19:31 did gyre and gimble:
> src/pulse/introspect.c | 74 +++++++++++++++++++++++++++++++++----------------
> src/pulse/introspect.h | 26 ++++++++++++-----
> src/utils/pactl.c | 10 +++---
> 3 files changed, 75 insertions(+), 35 deletions(-)
>
> New commits:
> commit 85e7fbc196f4424f68e530c2e3a01d9b941f293e
> Author: Luiz Augusto von Dentz <luiz.von.dentz at intel.com>
> Date: Mon Nov 4 19:41:22 2013 +0200
>
> introspect: Fix ABI break introduced by b98a2e1
>
> The size of pa_card_profile_info cannot change even if it just a field
> appended to end because each entry is appended to a contiguous memory
> and accessed by offset this may lead clients to access invalid data.
>
> To fix a new struct called pa_card_profile_info2 is introduced and shall
> be used for now on while pa_card_profile_info shall be considered
> deprecated but it is still mantained for backward compatibility.
>
> A new field called profiles2 is introduced to pa_card_info, this new field
> is an array of pointers to pa_card_profile_info2 so it should be possible
> to append new fields to the end of the pa_card_profile_info2 without
> breaking binary compatibility as the entries are not accessed by offset.
>
> diff --git a/src/pulse/introspect.c b/src/pulse/introspect.c
> index 45e0115..2d54fdb 100644
> --- a/src/pulse/introspect.c
> +++ b/src/pulse/introspect.c
> @@ -769,6 +769,15 @@ static void card_info_free(pa_card_info* i) {
>
> pa_xfree(i->profiles);
>
> + if (i->n_profiles) {
> + uint32_t j;
> +
> + for (j = 0; j < i->n_profiles; j++)
> + pa_xfree(i->profiles2[j]);
> +
> + pa_xfree(i->profiles2);
> + }
> +
> if (i->ports) {
> uint32_t j;
>
> @@ -776,6 +785,8 @@ static void card_info_free(pa_card_info* i) {
> if (i->ports[j]) {
> if (i->ports[j]->profiles)
> pa_xfree(i->ports[j]->profiles);
> + if (i->ports[j]->profiles2)
> + pa_xfree(i->ports[j]->profiles2);
> if (i->ports[j]->proplist)
> pa_proplist_free(i->ports[j]->proplist);
> }
> @@ -829,6 +840,7 @@ static int fill_card_port_info(pa_context *context, pa_tagstruct* t, pa_card_inf
>
> if (port->n_profiles > 0) {
> port->profiles = pa_xnew0(pa_card_profile_info*, i->n_profiles+1);
> + port->profiles2 = pa_xnew0(pa_card_profile_info2*, i->n_profiles+1);
>
> for (k = 0; k < port->n_profiles; k++) {
> const char* profilename;
> @@ -839,6 +851,7 @@ static int fill_card_port_info(pa_context *context, pa_tagstruct* t, pa_card_inf
> for (l = 0; l < i->n_profiles; l++) {
> if (pa_streq(i->profiles[l].name, profilename)) {
> port->profiles[k] = &i->profiles[l];
> + port->profiles2[k] = i->profiles2[l];
> break;
> }
> }
> @@ -857,6 +870,41 @@ static int fill_card_port_info(pa_context *context, pa_tagstruct* t, pa_card_inf
> return 0;
> }
>
> +static int fill_card_profile_info(pa_context *context, pa_tagstruct* t, pa_card_info* i) {
> + uint32_t j;
> +
> + i->profiles = pa_xnew0(pa_card_profile_info, i->n_profiles+1);
> + i->profiles2 = pa_xnew0(pa_card_profile_info2*, i->n_profiles+1);
> +
> + for (j = 0; j < i->n_profiles; j++) {
> + if (pa_tagstruct_gets(t, &i->profiles[j].name) < 0 ||
> + pa_tagstruct_gets(t, &i->profiles[j].description) < 0 ||
> + pa_tagstruct_getu32(t, &i->profiles[j].n_sinks) < 0 ||
> + pa_tagstruct_getu32(t, &i->profiles[j].n_sources) < 0 ||
> + pa_tagstruct_getu32(t, &i->profiles[j].priority) < 0)
> + return -PA_ERR_PROTOCOL;
> +
> + i->profiles2[j] = pa_xnew0(pa_card_profile_info2, 1);
> + i->profiles2[j]->name = i->profiles[j].name;
> + i->profiles2[j]->description = i->profiles[j].description;
> + i->profiles2[j]->n_sinks = i->profiles[j].n_sinks;
> + i->profiles2[j]->n_sources = i->profiles[j].n_sources;
> + i->profiles2[j]->priority = i->profiles[j].priority;
> + i->profiles2[j]->available = 1;
> +
> + if (context->version >= 29) {
> + uint32_t av;
> +
> + if (pa_tagstruct_getu32(t, &av) < 0)
> + return -PA_ERR_PROTOCOL;
> +
> + i->profiles2[j]->available = av;
> + }
> + }
> +
> + return 0;
> +}
> +
> static void context_get_card_info_callback(pa_pdispatch *pd, uint32_t command, uint32_t tag, pa_tagstruct *t, void *userdata) {
> pa_operation *o = userdata;
> int eol = 1;
> @@ -890,29 +938,8 @@ static void context_get_card_info_callback(pa_pdispatch *pd, uint32_t command, u
> goto fail;
>
> if (i.n_profiles > 0) {
> - i.profiles = pa_xnew0(pa_card_profile_info, i.n_profiles+1);
> -
> - for (j = 0; j < i.n_profiles; j++) {
> -
> - if (pa_tagstruct_gets(t, &i.profiles[j].name) < 0 ||
> - pa_tagstruct_gets(t, &i.profiles[j].description) < 0 ||
> - pa_tagstruct_getu32(t, &i.profiles[j].n_sinks) < 0 ||
> - pa_tagstruct_getu32(t, &i.profiles[j].n_sources) < 0 ||
> - pa_tagstruct_getu32(t, &i.profiles[j].priority) < 0)
> - goto fail;
> -
> - i.profiles[j].available = 1;
> - if (o->context->version >= 29) {
> - uint32_t av;
> - if (pa_tagstruct_getu32(t, &av) < 0)
> - goto fail;
> - i.profiles[j].available = av;
> - }
> - }
> -
> - /* Terminate with an extra NULL entry, just to make sure */
> - i.profiles[j].name = NULL;
> - i.profiles[j].description = NULL;
> + if (fill_card_profile_info(o->context, t, &i) < 0)
> + goto fail;
> }
>
> i.proplist = pa_proplist_new();
> @@ -929,6 +956,7 @@ static void context_get_card_info_callback(pa_pdispatch *pd, uint32_t command, u
> for (j = 0; j < i.n_profiles; j++)
> if (pa_streq(i.profiles[j].name, ap)) {
> i.active_profile = &i.profiles[j];
> + i.active_profile2 = i.profiles2[j];
> break;
> }
> }
> diff --git a/src/pulse/introspect.h b/src/pulse/introspect.h
> index f199a18..023b418 100644
> --- a/src/pulse/introspect.h
> +++ b/src/pulse/introspect.h
> @@ -443,22 +443,31 @@ pa_operation* pa_context_kill_client(pa_context *c, uint32_t idx, pa_context_suc
>
> /** @{ \name Cards */
>
> -/** Stores information about a specific profile of a card. Please
> - * note that this structure can be extended as part of evolutionary
> - * API updates at any time in any new release. \since 0.9.15 */
> +/** \deprecated Superseded by pa_card_profile_info2 \since 0.9.15 */
> typedef struct pa_card_profile_info {
> const char *name; /**< Name of this profile */
> const char *description; /**< Description of this profile */
> uint32_t n_sinks; /**< Number of sinks this profile would create */
> uint32_t n_sources; /**< Number of sources this profile would create */
> uint32_t priority; /**< The higher this value is, the more useful this profile is as a default. */
> +} pa_card_profile_info;
> +
> +/** Stores information about a specific profile of a card. Please
> + * note that this structure can be extended as part of evolutionary
> + * API updates at any time in any new release. \since 5.0 */
> +typedef struct pa_card_profile_info2 {
> + const char *name; /**< Name of this profile */
> + const char *description; /**< Description of this profile */
> + uint32_t n_sinks; /**< Number of sinks this profile would create */
> + uint32_t n_sources; /**< Number of sources this profile would create */
> + uint32_t priority; /**< The higher this value is, the more useful this profile is as a default. */
> int available;
> /**< Is this profile available? If this is zero, meaning "unavailable",
> * then it makes no sense to try to activate this profile. If this is
> * non-zero, it's still not a guarantee that activating the profile will
> * result in anything useful, it just means that the server isn't aware of
> * any reason why the profile would definitely be useless. \since 5.0 */
> -} pa_card_profile_info;
> +} pa_card_profile_info2;
>
> /** Stores information about a specific port of a card. Please
> * note that this structure can be extended as part of evolutionary
> @@ -470,9 +479,10 @@ typedef struct pa_card_port_info {
> int available; /**< A #pa_port_available enum, indicating availability status of this port. */
> int direction; /**< A #pa_direction enum, indicating the direction of this port. */
> uint32_t n_profiles; /**< Number of entries in profile array */
> - pa_card_profile_info** profiles; /**< Array of pointers to available profiles, or NULL. Array is terminated by an entry set to NULL. */
> + pa_card_profile_info** profiles; /**< \deprecated Superseded by profiles2 */
> pa_proplist *proplist; /**< Property list */
> int64_t latency_offset; /**< Latency offset of the port that gets added to the sink/source latency when the port is active. \since 3.0 */
> + pa_card_profile_info2** profiles2; /**< Array of pointers to available profiles, or NULL. Array is terminated by an entry set to NULL. */
> } pa_card_port_info;
>
> /** Stores information about cards. Please note that this structure
> @@ -484,11 +494,13 @@ typedef struct pa_card_info {
> uint32_t owner_module; /**< Index of the owning module, or PA_INVALID_INDEX. */
> const char *driver; /**< Driver name */
> uint32_t n_profiles; /**< Number of entries in profile array */
> - pa_card_profile_info* profiles; /**< Array of available profile, or NULL. Array is terminated by an entry with name set to NULL. Number of entries is stored in n_profiles. */
> - pa_card_profile_info* active_profile; /**< Pointer to active profile in the array, or NULL. */
> + pa_card_profile_info* profiles; /**< \deprecated Superseded by profiles2 */
> + pa_card_profile_info* active_profile; /**< \deprecated Superseded by active_profile2 */
> pa_proplist *proplist; /**< Property list */
> uint32_t n_ports; /**< Number of entries in port array */
> pa_card_port_info **ports; /**< Array of pointers to ports, or NULL. Array is terminated by an entry set to NULL. */
> + pa_card_profile_info2** profiles2; /**< Array of pointers to available profiles, or NULL. Array is terminated by an entry set to NULL. */
> + pa_card_profile_info2* active_profile2; /**< Pointer to active profile in the array, or NULL. */
> } pa_card_info;
>
> /** Callback prototype for pa_context_get_card_info_...() \since 0.9.15 */
> diff --git a/src/utils/pactl.c b/src/utils/pactl.c
> index df47caa..40e6689 100644
> --- a/src/utils/pactl.c
> +++ b/src/utils/pactl.c
> @@ -570,13 +570,13 @@ static void get_card_info_callback(pa_context *c, const pa_card_info *i, int is_
>
> pa_xfree(pl);
>
> - if (i->profiles) {
> - pa_card_profile_info *p;
> + if (i->n_profiles > 0) {
> + pa_card_profile_info2 **p;
>
> printf(_("\tProfiles:\n"));
> - for (p = i->profiles; p->name; p++)
> - printf("\t\t%s: %s (sinks: %u, sources: %u, priority: %u, available: %s)\n", p->name,
> - p->description, p->n_sinks, p->n_sources, p->priority, pa_yes_no(p->available));
> + for (p = i->profiles2; *p; p++)
> + printf("\t\t%s: %s (sinks: %u, sources: %u, priority: %u, available: %s)\n", (*p)->name,
> + (*p)->description, (*p)->n_sinks, (*p)->n_sources, (*p)->priority, pa_yes_no((*p)->available));
> }
>
> if (i->active_profile)
>
--
Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/
Day Job:
Tribalogic Limited http://www.tribalogic.net/
Open Source:
Mageia Contributor http://www.mageia.org/
PulseAudio Hacker http://www.pulseaudio.org/
Trac Hacker http://trac.edgewall.org/
More information about the pulseaudio-discuss
mailing list