[alsa-devel] [PATCH 0/3] ucm: miscellaneous fixes
While working on PulseAudio's UCM code, I noticed strange use of const in snd_use_case_get(). The third patch fixes that.
While working on the third patch, I also noticed strange code in execute_sequence(). The first patch fixes one obvious bug, but there was another issue which I didn't know how to fix, so I just added a FIXME comment instead (that's just as an RFC, I hope the discussion will reveal what the proper fix should be).
Tanu Kaskinen (3): ucm: fix variable mixup ucm: add a FIXME comment ucm: fix inappropriate use of const
include/use-case.h | 2 +- src/ucm/main.c | 34 +++++++++++++++++++++------------- 2 files changed, 22 insertions(+), 14 deletions(-)
I assume the intention was to use cdev1 for PlaybackCTL and cdev2 for CaptureCTL, but cdev1 was being used for both and cdev2 was not used for anything. --- src/ucm/main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/ucm/main.c b/src/ucm/main.c index 182f174..3924aee 100644 --- a/src/ucm/main.c +++ b/src/ucm/main.c @@ -308,7 +308,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr, uc_error("cdev is not defined!"); return err; } - err = get_value3(&cdev1, "CaptureCTL", + err = get_value3(&cdev2, "CaptureCTL", value_list1, value_list2, value_list3);
At Tue, 10 Feb 2015 22:42:32 +0200, Tanu Kaskinen wrote:
I assume the intention was to use cdev1 for PlaybackCTL and cdev2 for CaptureCTL, but cdev1 was being used for both and cdev2 was not used for anything.
Applied, thanks.
Takashi
src/ucm/main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/ucm/main.c b/src/ucm/main.c index 182f174..3924aee 100644 --- a/src/ucm/main.c +++ b/src/ucm/main.c @@ -308,7 +308,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr, uc_error("cdev is not defined!"); return err; }
err = get_value3(&cdev1, "CaptureCTL",
err = get_value3(&cdev2, "CaptureCTL", value_list1, value_list2, value_list3);-- 1.9.3
I'm pretty sure the current code will crash with some inputs, but I don't know what the original author intended this code to do, so I don't know how to fix it. --- src/ucm/main.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/ucm/main.c b/src/ucm/main.c index 3924aee..62bc374 100644 --- a/src/ucm/main.c +++ b/src/ucm/main.c @@ -317,6 +317,14 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr, uc_error("cdev is not defined!"); return err; } + /* FIXME: If cdev1 is NULL and cdev2 is not, + * then set cdev to cdev1... makes no sense! + * Also, what if both cdev1 and cdev2 are NULL? + * What should happen? Later in this function we + * call open_ctl(), which assumes non-NULL cdev, + * so leaving cdev to NULL here is not an + * option (or at least cdev has to be checked + * before calling open_ctl()). */ if (cdev1 == NULL || cdev2 == NULL || strcmp(cdev1, cdev2) == 0) { cdev = (char *)cdev1;
At Tue, 10 Feb 2015 22:42:33 +0200, Tanu Kaskinen wrote:
I'm pretty sure the current code will crash with some inputs, but I don't know what the original author intended this code to do, so I don't know how to fix it.
src/ucm/main.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/ucm/main.c b/src/ucm/main.c index 3924aee..62bc374 100644 --- a/src/ucm/main.c +++ b/src/ucm/main.c @@ -317,6 +317,14 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr, uc_error("cdev is not defined!"); return err; }
/* FIXME: If cdev1 is NULL and cdev2 is not,* then set cdev to cdev1... makes no sense!* Also, what if both cdev1 and cdev2 are NULL?* What should happen? Later in this function we* call open_ctl(), which assumes non-NULL cdev,* so leaving cdev to NULL here is not an* option (or at least cdev has to be checked* before calling open_ctl()). */ if (cdev1 == NULL || cdev2 == NULL || strcmp(cdev1, cdev2) == 0) { cdev = (char *)cdev1;
Ouch, the code is really buggy there. We must fix it instead of leaving FIXME.
I see there are multiple bugs (in addition to your patch#1):
- the error check is wrong, it should be compared with -ENOENT if (err < 0 && err != ENOENT)
This leaves cdev1 or cdev2 NULL as non-error.
- The intention of the code should be (as far as I understand): - if only one of cdev1 and cdev2 is defined, take it as cdev - if cdev1 and cdev2 are defined and have the same string, keep cdev1 and free cdev2 - in the rest cases, free both cdev1 and cdev2 without changing cdev
Takashi
On Wed, 2015-02-11 at 12:38 +0100, Takashi Iwai wrote:
At Tue, 10 Feb 2015 22:42:33 +0200, Tanu Kaskinen wrote:
I'm pretty sure the current code will crash with some inputs, but I don't know what the original author intended this code to do, so I don't know how to fix it.
src/ucm/main.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/ucm/main.c b/src/ucm/main.c index 3924aee..62bc374 100644 --- a/src/ucm/main.c +++ b/src/ucm/main.c @@ -317,6 +317,14 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr, uc_error("cdev is not defined!"); return err; }
/* FIXME: If cdev1 is NULL and cdev2 is not,* then set cdev to cdev1... makes no sense!* Also, what if both cdev1 and cdev2 are NULL?* What should happen? Later in this function we* call open_ctl(), which assumes non-NULL cdev,* so leaving cdev to NULL here is not an* option (or at least cdev has to be checked* before calling open_ctl()). */ if (cdev1 == NULL || cdev2 == NULL || strcmp(cdev1, cdev2) == 0) { cdev = (char *)cdev1;Ouch, the code is really buggy there. We must fix it instead of leaving FIXME.
I see there are multiple bugs (in addition to your patch#1):
the error check is wrong, it should be compared with -ENOENT if (err < 0 && err != ENOENT)
This leaves cdev1 or cdev2 NULL as non-error.
The intention of the code should be (as far as I understand):
- if only one of cdev1 and cdev2 is defined, take it as cdev
- if cdev1 and cdev2 are defined and have the same string, keep cdev1 and free cdev2
- in the rest cases, free both cdev1 and cdev2 without changing cdev
Regarding that last case, just leaving cdev unchanged is not a good idea. cdev will always be NULL if we don't set it here, and when we call open_ctl(), it must be non-NULL.
At Wed, 11 Feb 2015 13:44:40 +0200, Tanu Kaskinen wrote:
On Wed, 2015-02-11 at 12:38 +0100, Takashi Iwai wrote:
At Tue, 10 Feb 2015 22:42:33 +0200, Tanu Kaskinen wrote:
I'm pretty sure the current code will crash with some inputs, but I don't know what the original author intended this code to do, so I don't know how to fix it.
src/ucm/main.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/ucm/main.c b/src/ucm/main.c index 3924aee..62bc374 100644 --- a/src/ucm/main.c +++ b/src/ucm/main.c @@ -317,6 +317,14 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr, uc_error("cdev is not defined!"); return err; }
/* FIXME: If cdev1 is NULL and cdev2 is not,* then set cdev to cdev1... makes no sense!* Also, what if both cdev1 and cdev2 are NULL?* What should happen? Later in this function we* call open_ctl(), which assumes non-NULL cdev,* so leaving cdev to NULL here is not an* option (or at least cdev has to be checked* before calling open_ctl()). */ if (cdev1 == NULL || cdev2 == NULL || strcmp(cdev1, cdev2) == 0) { cdev = (char *)cdev1;Ouch, the code is really buggy there. We must fix it instead of leaving FIXME.
I see there are multiple bugs (in addition to your patch#1):
the error check is wrong, it should be compared with -ENOENT if (err < 0 && err != ENOENT)
This leaves cdev1 or cdev2 NULL as non-error.
The intention of the code should be (as far as I understand):
- if only one of cdev1 and cdev2 is defined, take it as cdev
- if cdev1 and cdev2 are defined and have the same string, keep cdev1 and free cdev2
- in the rest cases, free both cdev1 and cdev2 without changing cdev
Regarding that last case, just leaving cdev unchanged is not a good idea. cdev will always be NULL if we don't set it here, and when we call open_ctl(), it must be non-NULL.
Right. I *guess* if cdev1 != cdev2, it should bail out as an error.
Takashi
On Wed, 2015-02-11 at 14:01 +0100, Takashi Iwai wrote:
At Wed, 11 Feb 2015 13:44:40 +0200, Tanu Kaskinen wrote:
On Wed, 2015-02-11 at 12:38 +0100, Takashi Iwai wrote:
At Tue, 10 Feb 2015 22:42:33 +0200, Tanu Kaskinen wrote:
I'm pretty sure the current code will crash with some inputs, but I don't know what the original author intended this code to do, so I don't know how to fix it.
src/ucm/main.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/ucm/main.c b/src/ucm/main.c index 3924aee..62bc374 100644 --- a/src/ucm/main.c +++ b/src/ucm/main.c @@ -317,6 +317,14 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr, uc_error("cdev is not defined!"); return err; }
/* FIXME: If cdev1 is NULL and cdev2 is not,* then set cdev to cdev1... makes no sense!* Also, what if both cdev1 and cdev2 are NULL?* What should happen? Later in this function we* call open_ctl(), which assumes non-NULL cdev,* so leaving cdev to NULL here is not an* option (or at least cdev has to be checked* before calling open_ctl()). */ if (cdev1 == NULL || cdev2 == NULL || strcmp(cdev1, cdev2) == 0) { cdev = (char *)cdev1;Ouch, the code is really buggy there. We must fix it instead of leaving FIXME.
I see there are multiple bugs (in addition to your patch#1):
the error check is wrong, it should be compared with -ENOENT if (err < 0 && err != ENOENT)
This leaves cdev1 or cdev2 NULL as non-error.
The intention of the code should be (as far as I understand):
- if only one of cdev1 and cdev2 is defined, take it as cdev
- if cdev1 and cdev2 are defined and have the same string, keep cdev1 and free cdev2
- in the rest cases, free both cdev1 and cdev2 without changing cdev
Regarding that last case, just leaving cdev unchanged is not a good idea. cdev will always be NULL if we don't set it here, and when we call open_ctl(), it must be non-NULL.
Right. I *guess* if cdev1 != cdev2, it should bail out as an error.
Ok, I'll make a patch that will implement the described behaviour.
At Wed, 11 Feb 2015 15:34:06 +0200, Tanu Kaskinen wrote:
On Wed, 2015-02-11 at 14:01 +0100, Takashi Iwai wrote:
At Wed, 11 Feb 2015 13:44:40 +0200, Tanu Kaskinen wrote:
On Wed, 2015-02-11 at 12:38 +0100, Takashi Iwai wrote:
At Tue, 10 Feb 2015 22:42:33 +0200, Tanu Kaskinen wrote:
I'm pretty sure the current code will crash with some inputs, but I don't know what the original author intended this code to do, so I don't know how to fix it.
src/ucm/main.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/ucm/main.c b/src/ucm/main.c index 3924aee..62bc374 100644 --- a/src/ucm/main.c +++ b/src/ucm/main.c @@ -317,6 +317,14 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr, uc_error("cdev is not defined!"); return err; }
/* FIXME: If cdev1 is NULL and cdev2 is not,* then set cdev to cdev1... makes no sense!* Also, what if both cdev1 and cdev2 are NULL?* What should happen? Later in this function we* call open_ctl(), which assumes non-NULL cdev,* so leaving cdev to NULL here is not an* option (or at least cdev has to be checked* before calling open_ctl()). */ if (cdev1 == NULL || cdev2 == NULL || strcmp(cdev1, cdev2) == 0) { cdev = (char *)cdev1;Ouch, the code is really buggy there. We must fix it instead of leaving FIXME.
I see there are multiple bugs (in addition to your patch#1):
the error check is wrong, it should be compared with -ENOENT if (err < 0 && err != ENOENT)
This leaves cdev1 or cdev2 NULL as non-error.
The intention of the code should be (as far as I understand):
- if only one of cdev1 and cdev2 is defined, take it as cdev
- if cdev1 and cdev2 are defined and have the same string, keep cdev1 and free cdev2
- in the rest cases, free both cdev1 and cdev2 without changing cdev
Regarding that last case, just leaving cdev unchanged is not a good idea. cdev will always be NULL if we don't set it here, and when we call open_ctl(), it must be non-NULL.
Right. I *guess* if cdev1 != cdev2, it should bail out as an error.
Ok, I'll make a patch that will implement the described behaviour.
Thanks!
Takashi
On Wed, 2015-02-11 at 15:34 +0200, Tanu Kaskinen wrote:
On Wed, 2015-02-11 at 14:01 +0100, Takashi Iwai wrote:
At Wed, 11 Feb 2015 13:44:40 +0200, Tanu Kaskinen wrote:
On Wed, 2015-02-11 at 12:38 +0100, Takashi Iwai wrote:
At Tue, 10 Feb 2015 22:42:33 +0200, Tanu Kaskinen wrote:
I'm pretty sure the current code will crash with some inputs, but I don't know what the original author intended this code to do, so I don't know how to fix it.
src/ucm/main.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/ucm/main.c b/src/ucm/main.c index 3924aee..62bc374 100644 --- a/src/ucm/main.c +++ b/src/ucm/main.c @@ -317,6 +317,14 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr, uc_error("cdev is not defined!"); return err; }
/* FIXME: If cdev1 is NULL and cdev2 is not,* then set cdev to cdev1... makes no sense!* Also, what if both cdev1 and cdev2 are NULL?* What should happen? Later in this function we* call open_ctl(), which assumes non-NULL cdev,* so leaving cdev to NULL here is not an* option (or at least cdev has to be checked* before calling open_ctl()). */ if (cdev1 == NULL || cdev2 == NULL || strcmp(cdev1, cdev2) == 0) { cdev = (char *)cdev1;Ouch, the code is really buggy there. We must fix it instead of leaving FIXME.
I see there are multiple bugs (in addition to your patch#1):
the error check is wrong, it should be compared with -ENOENT if (err < 0 && err != ENOENT)
This leaves cdev1 or cdev2 NULL as non-error.
The intention of the code should be (as far as I understand):
- if only one of cdev1 and cdev2 is defined, take it as cdev
- if cdev1 and cdev2 are defined and have the same string, keep cdev1 and free cdev2
- in the rest cases, free both cdev1 and cdev2 without changing cdev
Regarding that last case, just leaving cdev unchanged is not a good idea. cdev will always be NULL if we don't set it here, and when we call open_ctl(), it must be non-NULL.
Right. I *guess* if cdev1 != cdev2, it should bail out as an error.
Ok, I'll make a patch that will implement the described behaviour.
It may also be worth adding a comment describing the cdev1/2 usage or maybe even rename them to make it obvious ?
Thanks
Liam
The caller is expected to free the string returned by snd_use_case_get(), but for some reason the string is marked as const, so the caller is forced to do type casting to get rid of compiler warnings. I don't see any reason for using const in snd_use_case_get(), so let's remove that.
This will cause warnings in application code, if applications currently pass const variables to the function, which they very likely do. I don't know if that's acceptable. If not, then it's too late to fix this bug... --- include/use-case.h | 2 +- src/ucm/main.c | 24 ++++++++++++------------ 2 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/include/use-case.h b/include/use-case.h index f30168f..28d786f 100644 --- a/include/use-case.h +++ b/include/use-case.h @@ -286,7 +286,7 @@ int snd_use_case_get_list(snd_use_case_mgr_t *uc_mgr, */ int snd_use_case_get(snd_use_case_mgr_t *uc_mgr, const char *identifier, - const char **value); + char **value);
/** * \brief Get current - integer diff --git a/src/ucm/main.c b/src/ucm/main.c index 62bc374..8fbf94f 100644 --- a/src/ucm/main.c +++ b/src/ucm/main.c @@ -40,9 +40,9 @@ * misc */
-static int get_value1(const char **value, struct list_head *value_list, +static int get_value1(char **value, struct list_head *value_list, const char *identifier); -static int get_value3(const char **value, +static int get_value3(char **value, const char *identifier, struct list_head *value_list1, struct list_head *value_list2, @@ -299,7 +299,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr, case SEQUENCE_ELEMENT_TYPE_CSET: case SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE: if (cdev == NULL) { - const char *cdev1 = NULL, *cdev2 = NULL; + char *cdev1 = NULL, *cdev2 = NULL; err = get_value3(&cdev1, "PlaybackCTL", value_list1, value_list2, @@ -313,7 +313,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr, value_list2, value_list3); if (err < 0 && err != ENOENT) { - free((char *)cdev1); + free(cdev1); uc_error("cdev is not defined!"); return err; } @@ -327,11 +327,11 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr, * before calling open_ctl()). */ if (cdev1 == NULL || cdev2 == NULL || strcmp(cdev1, cdev2) == 0) { - cdev = (char *)cdev1; - free((char *)cdev2); + cdev = cdev1; + free(cdev2); } else { - free((char *)cdev1); - free((char *)cdev2); + free(cdev1); + free(cdev2); } } if (ctl == NULL) { @@ -1233,7 +1233,7 @@ int snd_use_case_get_list(snd_use_case_mgr_t *uc_mgr, return err; }
-static int get_value1(const char **value, struct list_head *value_list, +static int get_value1(char **value, struct list_head *value_list, const char *identifier) { struct ucm_value *val; @@ -1254,7 +1254,7 @@ static int get_value1(const char **value, struct list_head *value_list, return -ENOENT; }
-static int get_value3(const char **value, +static int get_value3(char **value, const char *identifier, struct list_head *value_list1, struct list_head *value_list2, @@ -1284,7 +1284,7 @@ static int get_value3(const char **value, */ static int get_value(snd_use_case_mgr_t *uc_mgr, const char *identifier, - const char **value, + char **value, const char *mod_dev_name, const char *verb_name, int exact) @@ -1354,7 +1354,7 @@ static int get_value(snd_use_case_mgr_t *uc_mgr, */ int snd_use_case_get(snd_use_case_mgr_t *uc_mgr, const char *identifier, - const char **value) + char **value) { const char *slash1, *slash2, *mod_dev_after; const char *ident, *mod_dev, *verb;
At Tue, 10 Feb 2015 22:42:34 +0200, Tanu Kaskinen wrote:
The caller is expected to free the string returned by snd_use_case_get(), but for some reason the string is marked as const, so the caller is forced to do type casting to get rid of compiler warnings. I don't see any reason for using const in snd_use_case_get(), so let's remove that.
This will cause warnings in application code, if applications currently pass const variables to the function, which they very likely do. I don't know if that's acceptable. If not, then it's too late to fix this bug...
I'm afraid it's too late to change this.
Takashi
include/use-case.h | 2 +- src/ucm/main.c | 24 ++++++++++++------------ 2 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/include/use-case.h b/include/use-case.h index f30168f..28d786f 100644 --- a/include/use-case.h +++ b/include/use-case.h @@ -286,7 +286,7 @@ int snd_use_case_get_list(snd_use_case_mgr_t *uc_mgr, */ int snd_use_case_get(snd_use_case_mgr_t *uc_mgr, const char *identifier,
const char **value);
char **value);/**
- \brief Get current - integer
diff --git a/src/ucm/main.c b/src/ucm/main.c index 62bc374..8fbf94f 100644 --- a/src/ucm/main.c +++ b/src/ucm/main.c @@ -40,9 +40,9 @@
- misc
*/
-static int get_value1(const char **value, struct list_head *value_list, +static int get_value1(char **value, struct list_head *value_list, const char *identifier); -static int get_value3(const char **value, +static int get_value3(char **value, const char *identifier, struct list_head *value_list1, struct list_head *value_list2, @@ -299,7 +299,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr, case SEQUENCE_ELEMENT_TYPE_CSET: case SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE: if (cdev == NULL) {
const char *cdev1 = NULL, *cdev2 = NULL;
char *cdev1 = NULL, *cdev2 = NULL; err = get_value3(&cdev1, "PlaybackCTL", value_list1, value_list2,@@ -313,7 +313,7 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr, value_list2, value_list3); if (err < 0 && err != ENOENT) {
free((char *)cdev1);
free(cdev1); uc_error("cdev is not defined!"); return err; }@@ -327,11 +327,11 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr, * before calling open_ctl()). */ if (cdev1 == NULL || cdev2 == NULL || strcmp(cdev1, cdev2) == 0) {
cdev = (char *)cdev1;free((char *)cdev2);
cdev = cdev1;free(cdev2); } else {
free((char *)cdev1);free((char *)cdev2);
free(cdev1);free(cdev2); } } if (ctl == NULL) {@@ -1233,7 +1233,7 @@ int snd_use_case_get_list(snd_use_case_mgr_t *uc_mgr, return err; }
-static int get_value1(const char **value, struct list_head *value_list, +static int get_value1(char **value, struct list_head *value_list, const char *identifier) { struct ucm_value *val; @@ -1254,7 +1254,7 @@ static int get_value1(const char **value, struct list_head *value_list, return -ENOENT; }
-static int get_value3(const char **value, +static int get_value3(char **value, const char *identifier, struct list_head *value_list1, struct list_head *value_list2, @@ -1284,7 +1284,7 @@ static int get_value3(const char **value, */ static int get_value(snd_use_case_mgr_t *uc_mgr, const char *identifier,
const char **value,
char **value, const char *mod_dev_name, const char *verb_name, int exact)@@ -1354,7 +1354,7 @@ static int get_value(snd_use_case_mgr_t *uc_mgr, */ int snd_use_case_get(snd_use_case_mgr_t *uc_mgr, const char *identifier,
const char **value)
char **value){ const char *slash1, *slash2, *mod_dev_after; const char *ident, *mod_dev, *verb; -- 1.9.3
participants (3)
-
Liam Girdwood -
Takashi Iwai -
Tanu Kaskinen