[PATCH 0/9] scan-build fixes
These defects were identified by scan-build https://clang-analyzer.llvm.org/scan-build.html.
I hope that drawing attention to the problems helps even if you decide to fix some of them in different ways than I am proposing.
Happy holidays!
-Alex
Alex Henrie (9): conf: fix use after free in _snd_config_load_with_include ucm: fix bad frees in get_list0 and get_list20 rawmidi: fix memory leak in snd_rawmidi_virtual_open confmisc: fix memory leak in snd_func_concat timer: fix sizeof operator mismatch in snd_timer_query_hw_open pcm: remove dead assignments from snd_pcm_rate_(commit_area|grab_next_period) pcm_multi: remove dead assignment from _snd_pcm_multi_open pcm: fix undefined bit shift in bad_pcm_state conf: remove dead code from get_hexachar
include/pcm.h | 4 +++- src/conf.c | 13 ++++--------- src/confmisc.c | 2 +- src/pcm/pcm.c | 2 ++ src/pcm/pcm_local.h | 2 +- src/pcm/pcm_multi.c | 1 - src/pcm/pcm_rate.c | 2 -- src/rawmidi/rawmidi_virt.c | 3 ++- src/timer/timer_query_hw.c | 2 +- src/ucm/main.c | 4 ++-- 10 files changed, 16 insertions(+), 19 deletions(-)
Signed-off-by: Alex Henrie alexhenrie24@gmail.com --- src/conf.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/conf.c b/src/conf.c index 7df2b4e7..44d1bfde 100644 --- a/src/conf.c +++ b/src/conf.c @@ -1970,7 +1970,9 @@ int _snd_config_load_with_include(snd_config_t *config, snd_input_t *in, SNDERR("%s:%d:%d:%s", fd->name ? fd->name : "_toplevel_", fd->line, fd->column, str); goto _end; } - if (get_char(&input) != LOCAL_UNEXPECTED_EOF) { + err = get_char(&input); + fd = input.current; + if (err != LOCAL_UNEXPECTED_EOF) { SNDERR("%s:%d:%d:Unexpected }", fd->name ? fd->name : "", fd->line, fd->column); err = -EINVAL; goto _end;
On Sat, 26 Dec 2020 22:35:39 +0100, Alex Henrie wrote:
Signed-off-by: Alex Henrie alexhenrie24@gmail.com
Applied, thanks.
Takashi
Signed-off-by: Alex Henrie alexhenrie24@gmail.com --- src/ucm/main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/ucm/main.c b/src/ucm/main.c index 3871d5aa..754b967e 100644 --- a/src/ucm/main.c +++ b/src/ucm/main.c @@ -666,7 +666,7 @@ static int get_list0(struct list_head *list, } return cnt; __fail: - snd_use_case_free_list((const char **)res, cnt); + snd_use_case_free_list(*result, cnt); return -ENOMEM; }
@@ -724,7 +724,7 @@ static int get_list20(struct list_head *list, } return cnt; __fail: - snd_use_case_free_list((const char **)res, cnt); + snd_use_case_free_list(*result, cnt); return -ENOMEM; }
On Sat, 26 Dec 2020 22:35:40 +0100, Alex Henrie wrote:
Signed-off-by: Alex Henrie alexhenrie24@gmail.com
Applied, thanks.
Takashi
Signed-off-by: Alex Henrie alexhenrie24@gmail.com --- src/rawmidi/rawmidi_virt.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/rawmidi/rawmidi_virt.c b/src/rawmidi/rawmidi_virt.c index 2c4c27f5..884b8ff8 100644 --- a/src/rawmidi/rawmidi_virt.c +++ b/src/rawmidi/rawmidi_virt.c @@ -315,7 +315,7 @@ int snd_rawmidi_virtual_open(snd_rawmidi_t **inputp, snd_rawmidi_t **outputp, int merge, int mode) { int err; - snd_rawmidi_t *rmidi; + snd_rawmidi_t *rmidi = NULL; snd_rawmidi_virtual_t *virt = NULL; struct pollfd pfd;
@@ -392,6 +392,7 @@ int snd_rawmidi_virtual_open(snd_rawmidi_t **inputp, snd_rawmidi_t **outputp, free(*inputp); if (outputp) free(*outputp); + free(rmidi); return err; }
On Sat, 26 Dec 2020 22:35:41 +0100, Alex Henrie wrote:
Signed-off-by: Alex Henrie alexhenrie24@gmail.com
Applied, thanks.
Takashi
Signed-off-by: Alex Henrie alexhenrie24@gmail.com --- src/confmisc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/confmisc.c b/src/confmisc.c index eb8218c1..aece39c3 100644 --- a/src/confmisc.c +++ b/src/confmisc.c @@ -440,8 +440,8 @@ int snd_func_concat(snd_config_t **dst, snd_config_t *root, snd_config_t *src, err = snd_config_get_id(src, &id); if (err >= 0) err = snd_config_imake_string(dst, id, res); - free(res); __error: + free(res); return err; } #ifndef DOC_HIDDEN
On Sat, 26 Dec 2020 22:35:42 +0100, Alex Henrie wrote:
Signed-off-by: Alex Henrie alexhenrie24@gmail.com
src/confmisc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/confmisc.c b/src/confmisc.c index eb8218c1..aece39c3 100644 --- a/src/confmisc.c +++ b/src/confmisc.c @@ -440,8 +440,8 @@ int snd_func_concat(snd_config_t **dst, snd_config_t *root, snd_config_t *src, err = snd_config_get_id(src, &id); if (err >= 0) err = snd_config_imake_string(dst, id, res);
- free(res); __error:
- free(res); return err;
} #ifndef DOC_HIDDEN
I guess this would lead to the double-free at the error path after realloc() that already freed res before the goto line. Care to fix it and resubmit this one?
thanks,
Takashi
On Sun, Dec 27, 2020 at 1:26 AM Takashi Iwai tiwai@suse.de wrote:
I guess this would lead to the double-free at the error path after realloc() that already freed res before the goto line. Care to fix it and resubmit this one?
Thank you for catching that! I just sent a v2 of this patch.
-Alex
Signed-off-by: Alex Henrie alexhenrie24@gmail.com --- src/timer/timer_query_hw.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/timer/timer_query_hw.c b/src/timer/timer_query_hw.c index dad228c8..d8bac6e7 100644 --- a/src/timer/timer_query_hw.c +++ b/src/timer/timer_query_hw.c @@ -104,7 +104,7 @@ int snd_timer_query_hw_open(snd_timer_query_t **handle, const char *name, int mo close(fd); return -SND_ERROR_INCOMPATIBLE_VERSION; } - tmr = (snd_timer_query_t *) calloc(1, sizeof(snd_timer_t)); + tmr = (snd_timer_query_t *) calloc(1, sizeof(snd_timer_query_t)); if (tmr == NULL) { close(fd); return -ENOMEM;
On Sat, 26 Dec 2020 22:35:43 +0100, Alex Henrie wrote:
Signed-off-by: Alex Henrie alexhenrie24@gmail.com
Applied, thanks.
Takashi
Signed-off-by: Alex Henrie alexhenrie24@gmail.com --- src/pcm/pcm_rate.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c index 5bf7dbb9..dc38e95e 100644 --- a/src/pcm/pcm_rate.c +++ b/src/pcm/pcm_rate.c @@ -746,7 +746,6 @@ static int snd_pcm_rate_commit_area(snd_pcm_t *pcm, snd_pcm_rate_t *rate, if (result < 0) return result; __partial: - xfer = 0; cont = slave_frames; if (cont > slave_size) cont = slave_size; @@ -846,7 +845,6 @@ static int snd_pcm_rate_grab_next_period(snd_pcm_t *pcm, snd_pcm_uframes_t hw_of if (result < 0) return result; __partial: - xfer = 0; cont = slave_frames; if (cont > rate->gen.slave->period_size) cont = rate->gen.slave->period_size;
On Sat, 26 Dec 2020 22:35:44 +0100, Alex Henrie wrote:
Signed-off-by: Alex Henrie alexhenrie24@gmail.com
Applied, thanks.
Takashi
Signed-off-by: Alex Henrie alexhenrie24@gmail.com --- src/pcm/pcm_multi.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/src/pcm/pcm_multi.c b/src/pcm/pcm_multi.c index 53c414d5..5fa09b9b 100644 --- a/src/pcm/pcm_multi.c +++ b/src/pcm/pcm_multi.c @@ -1323,7 +1323,6 @@ int _snd_pcm_multi_open(snd_pcm_t **pcmp, const char *name, err = -ENOMEM; goto _free; } - idx = 0; for (idx = 0; idx < channels_count; ++idx) channels_sidx[idx] = -1; idx = 0;
On Sat, 26 Dec 2020 22:35:45 +0100, Alex Henrie wrote:
Signed-off-by: Alex Henrie alexhenrie24@gmail.com
Applied, thanks.
Takashi
Signed-off-by: Alex Henrie alexhenrie24@gmail.com --- include/pcm.h | 4 +++- src/pcm/pcm.c | 2 ++ src/pcm/pcm_local.h | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/include/pcm.h b/include/pcm.h index e300b951..c6c5d8f8 100644 --- a/include/pcm.h +++ b/include/pcm.h @@ -307,7 +307,9 @@ typedef enum _snd_pcm_state { SND_PCM_STATE_SUSPENDED, /** Hardware is disconnected */ SND_PCM_STATE_DISCONNECTED, - SND_PCM_STATE_LAST = SND_PCM_STATE_DISCONNECTED, + /** State cannot be queried */ + SND_PCM_STATE_UNKNOWN, + SND_PCM_STATE_LAST = SND_PCM_STATE_UNKNOWN, /** Private - used internally in the library - do not use*/ SND_PCM_STATE_PRIVATE1 = 1024 } snd_pcm_state_t; diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c index 24030b31..5fafc2a0 100644 --- a/src/pcm/pcm.c +++ b/src/pcm/pcm.c @@ -680,6 +680,8 @@ static int pcm_state_to_error(snd_pcm_state_t state) return -ESTRPIPE; case SND_PCM_STATE_DISCONNECTED: return -ENODEV; + case SND_PCM_STATE_UNKNOWN: + return -ENOSYS; default: return 0; } diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h index fe77e50d..04f89623 100644 --- a/src/pcm/pcm_local.h +++ b/src/pcm/pcm_local.h @@ -444,7 +444,7 @@ static inline int __snd_pcm_start(snd_pcm_t *pcm) static inline snd_pcm_state_t __snd_pcm_state(snd_pcm_t *pcm) { if (!pcm->fast_ops->state) - return -ENOSYS; + return SND_PCM_STATE_UNKNOWN; return pcm->fast_ops->state(pcm->fast_op_arg); }
On Sat, 26 Dec 2020 22:35:46 +0100, Alex Henrie wrote:
Signed-off-by: Alex Henrie alexhenrie24@gmail.com
include/pcm.h | 4 +++- src/pcm/pcm.c | 2 ++ src/pcm/pcm_local.h | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/include/pcm.h b/include/pcm.h index e300b951..c6c5d8f8 100644 --- a/include/pcm.h +++ b/include/pcm.h @@ -307,7 +307,9 @@ typedef enum _snd_pcm_state { SND_PCM_STATE_SUSPENDED, /** Hardware is disconnected */ SND_PCM_STATE_DISCONNECTED,
- SND_PCM_STATE_LAST = SND_PCM_STATE_DISCONNECTED,
- /** State cannot be queried */
- SND_PCM_STATE_UNKNOWN,
- SND_PCM_STATE_LAST = SND_PCM_STATE_UNKNOWN, /** Private - used internally in the library - do not use*/ SND_PCM_STATE_PRIVATE1 = 1024
} snd_pcm_state_t;
We can't add a random new state here. If any, such a thing has to be defined locally, but this would also break ABI. So, I don't think adding a new state is the right fix.
diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c index 24030b31..5fafc2a0 100644 --- a/src/pcm/pcm.c +++ b/src/pcm/pcm.c @@ -680,6 +680,8 @@ static int pcm_state_to_error(snd_pcm_state_t state) return -ESTRPIPE; case SND_PCM_STATE_DISCONNECTED: return -ENODEV;
- case SND_PCM_STATE_UNKNOWN:
return -ENOSYS;diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h index fe77e50d..04f89623 100644 --- a/src/pcm/pcm_local.h +++ b/src/pcm/pcm_local.h @@ -444,7 +444,7 @@ static inline int __snd_pcm_start(snd_pcm_t *pcm) static inline snd_pcm_state_t __snd_pcm_state(snd_pcm_t *pcm) { if (!pcm->fast_ops->state)
return -ENOSYS;
return SND_PCM_STATE_UNKNOWN;
We need either to handle a special error value in all places calling __snd_pcm_state() or to just return SND_PCM_STATE_XRUN or such instead here, IMO.
thanks,
Takashi
Dne 27. 12. 20 v 9:34 Takashi Iwai napsal(a):
We need either to handle a special error value in all places calling __snd_pcm_state() or to just return SND_PCM_STATE_XRUN or such instead here, IMO.
I think that SND_PCM_STATE_OPEN is more appropriate here. If the state callback is not defined, the state management is screwed anyway. The other functions will return an error (because they depend on the state management), so it's safe. I applied this change to repo.
Jaroslav
On Sun, Dec 27, 2020 at 5:36 AM Jaroslav Kysela perex@perex.cz wrote:
Dne 27. 12. 20 v 9:34 Takashi Iwai napsal(a):
We need either to handle a special error value in all places calling __snd_pcm_state() or to just return SND_PCM_STATE_XRUN or such instead here, IMO.
I think that SND_PCM_STATE_OPEN is more appropriate here. If the state callback is not defined, the state management is screwed anyway. The other functions will return an error (because they depend on the state management), so it's safe. I applied this change to repo.
Thank you for fixing this properly!
-Alex
Signed-off-by: Alex Henrie alexhenrie24@gmail.com --- src/conf.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/src/conf.c b/src/conf.c index 44d1bfde..970ad643 100644 --- a/src/conf.c +++ b/src/conf.c @@ -877,16 +877,9 @@ static int get_nonwhite(input_t *input)
static inline int get_hexachar(input_t *input) { - int c, num = 0; - + int c; c = get_char(input); - if (c >= '0' && c <= '9') num |= (c - '0') << 4; - else if (c >= 'a' && c <= 'f') num |= (c - 'a') << 4; - else if (c >= 'A' && c <= 'F') num |= (c - 'A') << 4; c = get_char(input); - if (c >= '0' && c <= '9') num |= (c - '0') << 0; - else if (c >= 'a' && c <= 'f') num |= (c - 'a') << 0; - else if (c >= 'A' && c <= 'F') num |= (c - 'A') << 0; return c; }
On Sat, 26 Dec 2020 22:35:47 +0100, Alex Henrie wrote:
Signed-off-by: Alex Henrie alexhenrie24@gmail.com
src/conf.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/src/conf.c b/src/conf.c index 44d1bfde..970ad643 100644 --- a/src/conf.c +++ b/src/conf.c @@ -877,16 +877,9 @@ static int get_nonwhite(input_t *input)
static inline int get_hexachar(input_t *input) {
- int c, num = 0;
- int c; c = get_char(input);
- if (c >= '0' && c <= '9') num |= (c - '0') << 4;
- else if (c >= 'a' && c <= 'f') num |= (c - 'a') << 4;
- else if (c >= 'A' && c <= 'F') num |= (c - 'A') << 4; c = get_char(input);
- if (c >= '0' && c <= '9') num |= (c - '0') << 0;
- else if (c >= 'a' && c <= 'f') num |= (c - 'a') << 0;
- else if (c >= 'A' && c <= 'F') num |= (c - 'A') << 0; return c;
The current code is obviously wrong and the suggested fix goes even to a wronger direction :) The function should return num instead.
I wonder how this did't hit any problem, so far. Maybe 0x prefix was rarely used, fortunately.
thanks,
Takashi
Dne 27. 12. 20 v 9:37 Takashi Iwai napsal(a):
On Sat, 26 Dec 2020 22:35:47 +0100, Alex Henrie wrote:
Signed-off-by: Alex Henrie alexhenrie24@gmail.com
src/conf.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/src/conf.c b/src/conf.c index 44d1bfde..970ad643 100644 --- a/src/conf.c +++ b/src/conf.c @@ -877,16 +877,9 @@ static int get_nonwhite(input_t *input)
static inline int get_hexachar(input_t *input) {
- int c, num = 0;
- int c; c = get_char(input);
- if (c >= '0' && c <= '9') num |= (c - '0') << 4;
- else if (c >= 'a' && c <= 'f') num |= (c - 'a') << 4;
- else if (c >= 'A' && c <= 'F') num |= (c - 'A') << 4; c = get_char(input);
- if (c >= '0' && c <= '9') num |= (c - '0') << 0;
- else if (c >= 'a' && c <= 'f') num |= (c - 'a') << 0;
- else if (c >= 'A' && c <= 'F') num |= (c - 'A') << 0; return c;
The current code is obviously wrong and the suggested fix goes even to a wronger direction :) The function should return num instead.
I wonder how this did't hit any problem, so far. Maybe 0x prefix was rarely used, fortunately.
It's a bit recent code. I fixed the return value now. It's for \xFF not for 0xFF prefix. Thank you for your investigation, Alex.
Jaroslav
On Sun, Dec 27, 2020 at 5:25 AM Jaroslav Kysela perex@perex.cz wrote:
Dne 27. 12. 20 v 9:37 Takashi Iwai napsal(a):
The current code is obviously wrong and the suggested fix goes even to a wronger direction :) The function should return num instead.
I wonder how this did't hit any problem, so far. Maybe 0x prefix was rarely used, fortunately.
It's a bit recent code. I fixed the return value now. It's for \xFF not for 0xFF prefix. Thank you for your investigation, Alex.
Thank you for fixing this properly!
-Alex
participants (3)
-
Alex Henrie -
Jaroslav Kysela -
Takashi Iwai