| Message ID | 1423600954-8751-3-git-send-email-tanu.kaskinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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;