[pulseaudio-discuss] [PATCH] introspect: Fix ABI break introduce by b98a2e1
Tanu Kaskinen
tanu.kaskinen at linux.intel.com
Mon Nov 4 15:54:10 CET 2013
On Mon, 2013-11-04 at 16:13 +0200, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz at intel.com>
>
> The size of pa_card_profile_info cannot change even if it just a field
> appended to end because it is appended to an array 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.
> ---
> src/pulse/introspect.c | 88 +++++++++++++++++++++++++++++++++++---------------
> src/pulse/introspect.h | 14 +++++++-
> src/utils/pactl.c | 4 +--
> 3 files changed, 77 insertions(+), 29 deletions(-)
Thanks for the patch! Review comments inline.
> diff --git a/src/pulse/introspect.c b/src/pulse/introspect.c
> index 45e0115..266f03f 100644
> --- a/src/pulse/introspect.c
> +++ b/src/pulse/introspect.c
> @@ -768,6 +768,7 @@ static void card_info_free(pa_card_info* i) {
> pa_proplist_free(i->proplist);
>
> pa_xfree(i->profiles);
> + pa_xfree(i->profiles2);
>
> if (i->ports) {
> uint32_t j;
> @@ -776,6 +777,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);
> }
> @@ -837,7 +840,12 @@ static int fill_card_port_info(pa_context *context, pa_tagstruct* t, pa_card_inf
> return -PA_ERR_PROTOCOL;
>
> for (l = 0; l < i->n_profiles; l++) {
> - if (pa_streq(i->profiles[l].name, profilename)) {
> + if (context->version >= 29) {
This shouldn't be protocol version dependent code. Applications are
going to use profiles2 regardless of the negotiated protocol version.
> + if (pa_streq(i->profiles2[l].name, profilename)) {
> + port->profiles2[k] = &i->profiles2[l];
> + break;
> + }
> + } else if (pa_streq(i->profiles[l].name, profilename)) {
> port->profiles[k] = &i->profiles[l];
> break;
> }
> @@ -857,6 +865,49 @@ 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);
> +
> + if (context->version >= 29)
> + i->profiles2 = pa_xnew0(pa_card_profile_info2, i->n_profiles+1);
Shouldn't be protocol version dependent.
> +
> + 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;
> +
> + if (context->version >= 29) {
> + uint32_t av;
> +
> + 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;
> +
> + if (pa_tagstruct_getu32(t, &av) < 0)
> + return -PA_ERR_PROTOCOL;
This is the only protocol version dependent bit here. Setting the name,
description etc. for profiles2[j] should be done always.
> +
> + i->profiles2[j].available = av;
> + }
> + }
> +
> + /* Terminate with an extra NULL entry, just to make sure */
> + i->profiles[j].name = NULL;
> + i->profiles[j].description = NULL;
> +
> + if (context->version >= 29)
> + i->profiles2[j].name = NULL;
> + i->profiles2[j].description = NULL;
Not protocol version dependent (also, missing curly braces).
Hmm, all this NULL setting is redundant anyway, because pa_xnew0()
already zeroes the memory.
> +
> + 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 +941,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;
This default initialization to 1 is missing from the new code.
> - 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();
> @@ -926,11 +956,17 @@ static void context_get_card_info_callback(pa_pdispatch *pd, uint32_t command, u
> }
>
> if (ap) {
> - for (j = 0; j < i.n_profiles; j++)
> - if (pa_streq(i.profiles[j].name, ap)) {
> + for (j = 0; j < i.n_profiles; j++) {
> + if (o->context->version >= 29) {
Not protocol version dependent.
> + if (pa_streq(i.profiles2[j].name, ap)) {
> + i.active_profile2 = &i.profiles2[j];
> + break;
> + }
> + } else if (pa_streq(i.profiles[j].name, ap)) {
> i.active_profile = &i.profiles[j];
> break;
> }
> + }
> }
>
> if (o->context->version >= 26) {
> diff --git a/src/pulse/introspect.h b/src/pulse/introspect.h
> index f199a18..8040eb3 100644
> --- a/src/pulse/introspect.h
> +++ b/src/pulse/introspect.h
> @@ -452,13 +452,22 @@ typedef struct pa_card_profile_info {
> 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;
The documentation of pa_card_profile_info should state that the struct
has been superseded by pa_card_profile_info2. Also, the sentence "this
structure can be extended as part of evolutionary API updates at any
time in any new release" is not true for this struct, so that text can
be removed.
> +
> +/** Stores extended information about a specific profile of a card. \since 5.0 */
The documentation should contain the standard note: "Please note that
this structure can be extended as part of evolutionary API updates at
any time in any new release."
Also, I think the word "extended" is not necessary. Yes, it's true that
the information is extended compared to pa_card_profile_info, but
developers should ignore pa_card_profile_info altogether, so it's not
good to make the implicit reference to pa_card_profile_info here.
> +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
> @@ -471,6 +480,7 @@ typedef struct pa_card_port_info {
> 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_info2** profiles2; /**< Array of pointers to available profiles, or NULL. Array is terminated by an entry set to NULL. */
New fields can only be added to the end of the struct. Doing otherwise
breaks binary compatibility.
> 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_port_info;
> @@ -485,7 +495,9 @@ typedef struct pa_card_info {
> 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_info2* profiles2; /**< 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_info2* active_profile2; /**< Pointer to active profile in the array, or NULL. */
The new fields should be added to the end of the struct.
Also, the new array should be an array of pointers, not an array of
structs. This same mistake with the old profiles field is exactly the
thing that caused all this trouble in the first place.
--
Tanu
More information about the pulseaudio-discuss
mailing list