Iterating over texts in tplg_decode_control_enum1 was an infinite loop, it needed to be fixed. Parsing values was removed since they are not added to the UML file. Also texts are separate section and therefore it needs to be added as separate element.

Signed-off-by: Piotr Maziarz piotrx.maziarz@linux.intel.com --- src/topology/ctl.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/src/topology/ctl.c b/src/topology/ctl.c index c8c7e94..24eadc8 100644 --- a/src/topology/ctl.c +++ b/src/topology/ctl.c @@ -1088,11 +1088,19 @@ int tplg_add_enum(snd_tplg_t *tplg, struct snd_tplg_enum_template *enum_ctl, }

if (enum_ctl->texts != NULL) { + struct tplg_elem *texts = tplg_elem_new_common(tplg, NULL, + enum_ctl->hdr.name, SND_TPLG_TYPE_TEXT); + + texts->texts->num_items = num_items; for (i = 0; i < num_items; i++) { - if (enum_ctl->texts[i] != NULL) - snd_strlcpy(ec->texts[i], enum_ctl->texts[i], - SNDRV_CTL_ELEM_ID_NAME_MAXLEN); + if (!enum_ctl->texts[i]) + continue; + snd_strlcpy(ec->texts[i], enum_ctl->texts[i], + SNDRV_CTL_ELEM_ID_NAME_MAXLEN); + snd_strlcpy(texts->texts->items[i], enum_ctl->texts[i], + SNDRV_CTL_ELEM_ID_NAME_MAXLEN); } + tplg_ref_add(elem, SND_TPLG_TYPE_TEXT, enum_ctl->hdr.name); }

if (enum_ctl->values != NULL) { @@ -1367,11 +1375,8 @@ int tplg_decode_control_enum1(snd_tplg_t *tplg, et->texts = tplg_calloc(heap, sizeof(char *) * ec->items); if (!et->texts) return -ENOMEM; - for (i = 0; ec->items; i++) { - unsigned int j = i * sizeof(int) * ENUM_VAL_SIZE; + for (i = 0; i < ec->items; i++) et->texts[i] = ec->texts[i]; - et->values[i] = (int *)&ec->values[j]; - } }

et->map = tplg_calloc(heap, sizeof(struct snd_tplg_channel_map_template));

Size constraints are always checked before invoking tplg_decode_control_enum1. There is no need to validate it twice. Alos moved debug print about size to invoking function, since now it's it responsibility to check size.

Signed-off-by: Piotr Maziarz piotrx.maziarz@linux.intel.com --- src/topology/ctl.c | 19 +++++-------------- src/topology/dapm.c | 3 +-- src/topology/tplg_local.h | 2 +- 3 files changed, 7 insertions(+), 17 deletions(-)

diff --git a/src/topology/ctl.c b/src/topology/ctl.c index 24eadc8..b5e3e34 100644 --- a/src/topology/ctl.c +++ b/src/topology/ctl.c @@ -1335,22 +1335,10 @@ int tplg_decode_control_enum1(snd_tplg_t *tplg, struct list_head *heap, struct snd_tplg_enum_template *et, size_t pos, - void *bin, size_t size) + struct snd_soc_tplg_enum_control *ec) { - struct snd_soc_tplg_enum_control *ec = bin; int i;

- if (size < sizeof(*ec)) { - SNDERR("enum: small size %d", size); - return -EINVAL; - } - - tplg_log(tplg, 'D', pos, "enum: size %d private size %d", - ec->size, ec->priv.size); - if (size != ec->size + ec->priv.size) { - SNDERR("enum: unexpected element size %d", size); - return -EINVAL; - } if (ec->num_channels > SND_TPLG_MAX_CHAN || ec->num_channels > SND_SOC_TPLG_MAX_CHAN) { SNDERR("enum: unexpected channel count %d", ec->num_channels); @@ -1425,7 +1413,10 @@ next: return -EINVAL; }

- err = tplg_decode_control_enum1(tplg, &heap, &et, pos, bin, size); + tplg_log(tplg, 'D', pos, "enum: size %d private size %d", + ec->size, ec->priv.size); + + err = tplg_decode_control_enum1(tplg, &heap, &et, pos, ec); if (err >= 0) { t.enum_ctl = &et; err = snd_tplg_add_object(tplg, &t); diff --git a/src/topology/dapm.c b/src/topology/dapm.c index cd1a877..73a9390 100644 --- a/src/topology/dapm.c +++ b/src/topology/dapm.c @@ -972,8 +972,7 @@ next: err = -EINVAL; goto retval; } - err = tplg_decode_control_enum1(tplg, &heap, et, pos, - bin, size2); + err = tplg_decode_control_enum1(tplg, &heap, et, pos, ec); break; case SND_SOC_TPLG_TYPE_BYTES: bt = tplg_calloc(&heap, sizeof(*bt)); diff --git a/src/topology/tplg_local.h b/src/topology/tplg_local.h index 5ace0d1..acb01a8 100644 --- a/src/topology/tplg_local.h +++ b/src/topology/tplg_local.h @@ -398,7 +398,7 @@ int tplg_decode_control_enum1(snd_tplg_t *tplg, struct list_head *heap, struct snd_tplg_enum_template *et, size_t pos, - void *bin, size_t size); + struct snd_soc_tplg_enum_control *ec); int tplg_decode_control_enum(snd_tplg_t *tplg, size_t pos, struct snd_soc_tplg_hdr *hdr, void *bin, size_t size);

Not printing this field makes data loss while converting from binary to UML.

Signed-off-by: Piotr Maziarz piotrx.maziarz@linux.intel.com --- src/topology/pcm.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/src/topology/pcm.c b/src/topology/pcm.c index db40114..49c5eab 100644 --- a/src/topology/pcm.c +++ b/src/topology/pcm.c @@ -604,6 +604,9 @@ int tplg_save_stream_caps(snd_tplg_t *tplg ATTRIBUTE_UNUSED, if (err >= 0 && sc->buffer_size_max) err = tplg_save_printf(dst, pfx, "\tbuffer_size_max %u\n", sc->buffer_size_max); + if (err >= 0 && sc->sig_bits) + err = tplg_save_printf(dst, pfx, "\tsig_bits %u\n", + sc->sig_bits); if (err >= 0) err = tplg_save_printf(dst, pfx, "}\n"); return err;

Some fields can exceed size limit, e.g. private data has no size restriction. Therefore it needs to be dynamically increased.

Signed-off-by: Piotr Maziarz piotrx.maziarz@linux.intel.com --- src/topology/save.c | 36 ++++++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/src/topology/save.c b/src/topology/save.c index 4ecf86c..d6ee8b6 100644 --- a/src/topology/save.c +++ b/src/topology/save.c @@ -19,22 +19,43 @@ #include "tplg_local.h"

#define SAVE_ALLOC_SHIFT (13) /* 8192 bytes */ +#define PRINT_BUF_SIZE (1024) +#define PRINT_BUF_SIZE_MAX (1024 * 1024)

int tplg_save_printf(char **dst, const char *pfx, const char *fmt, ...) { va_list va; - char buf[1024], *s; - size_t n, l, t, pl; + char *buf, *s; + size_t n, alloc_size = PRINT_BUF_SIZE, l, t, pl; + int ret = 0; + + buf = malloc(alloc_size); + if (!buf) + return -ENOMEM;

if (pfx == NULL) pfx = "";

+again: va_start(va, fmt); - n = vsnprintf(buf, sizeof(buf), fmt, va); + n = vsnprintf(buf, alloc_size, fmt, va); va_end(va);

- if (n >= sizeof(buf)) - return -EOVERFLOW; + if (n >= PRINT_BUF_SIZE_MAX) { + ret = -EOVERFLOW; + goto end; + } + + if (n >= alloc_size) { + char *tmp = realloc(buf, n + 1); + if (!tmp) { + ret = -ENOMEM; + goto end; + } + buf = tmp; + alloc_size = n + 1; + goto again; + }

pl = strlen(pfx); l = *dst ? strlen(*dst) : 0; @@ -47,7 +68,8 @@ int tplg_save_printf(char **dst, const char *pfx, const char *fmt, ...) if (s == NULL) { free(*dst); *dst = NULL; - return -ENOMEM; + ret = -ENOMEM; + goto end; } } else { s = *dst; @@ -57,6 +79,8 @@ int tplg_save_printf(char **dst, const char *pfx, const char *fmt, ...) strcpy(s + l, pfx); strcpy(s + l + pl, buf); *dst = s; +end: + free(buf); return 0; }

On 6/25/20 6:03 AM, Piotr Maziarz wrote:

Iterating over texts in tplg_decode_control_enum1 was an infinite loop,

that should probably be a separate fix?

it needed to be fixed. Parsing values was removed since they are not added to the UML file.

What does this mean? first time I hear about UML for topology.

Also texts are separate section and therefore it needs to be added as separate element.

separate patch?

Signed-off-by: Piotr Maziarz piotrx.maziarz@linux.intel.com

src/topology/ctl.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/src/topology/ctl.c b/src/topology/ctl.c index c8c7e94..24eadc8 100644 --- a/src/topology/ctl.c +++ b/src/topology/ctl.c @@ -1088,11 +1088,19 @@ int tplg_add_enum(snd_tplg_t *tplg, struct snd_tplg_enum_template *enum_ctl, }

if (enum_ctl->texts != NULL) {

• struct tplg_elem *texts = tplg_elem_new_common(tplg, NULL,
  

• 				enum_ctl->hdr.name, SND_TPLG_TYPE_TEXT);
  

• texts->texts->num_items = num_items;
  

  for (i = 0; i < num_items; i++) {

• 	if (enum_ctl->texts[i] != NULL)
  

• 		snd_strlcpy(ec->texts[i], enum_ctl->texts[i],
  

• 			    SNDRV_CTL_ELEM_ID_NAME_MAXLEN);
  

• 	if (!enum_ctl->texts[i])
  

• 		continue;
  

• 	snd_strlcpy(ec->texts[i], enum_ctl->texts[i],
  

• 		    SNDRV_CTL_ELEM_ID_NAME_MAXLEN);
  

• 	snd_strlcpy(texts->texts->items[i], enum_ctl->texts[i],
  

• 		    SNDRV_CTL_ELEM_ID_NAME_MAXLEN);
  

  }

• tplg_ref_add(elem, SND_TPLG_TYPE_TEXT, enum_ctl->hdr.name);
  

  }

  if (enum_ctl->values != NULL) {

@@ -1367,11 +1375,8 @@ int tplg_decode_control_enum1(snd_tplg_t *tplg, et->texts = tplg_calloc(heap, sizeof(char *) * ec->items); if (!et->texts) return -ENOMEM;

• for (i = 0; ec->items; i++) {
  

• 	unsigned int j = i * sizeof(int) * ENUM_VAL_SIZE;
  

• for (i = 0; i < ec->items; i++)
  

This is the infinite loop, this should not be buried in unrelated changes.

	et->texts[i] = ec->texts[i];

• 	et->values[i] = (int *)&ec->values[j];
  

• }
  

  }

  et->map = tplg_calloc(heap, sizeof(struct snd_tplg_channel_map_template));

On 2020-06-25 16:31, Pierre-Louis Bossart wrote:

On 6/25/20 6:03 AM, Piotr Maziarz wrote:

...

Some fields can exceed size limit, e.g. private data has no size restriction. Therefore it needs to be dynamically increased.

Signed-off-by: Piotr Maziarz piotrx.maziarz@linux.intel.com

src/topology/save.c | 36 ++++++++++++++++++++++++++++++------   1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/src/topology/save.c b/src/topology/save.c index 4ecf86c..d6ee8b6 100644 --- a/src/topology/save.c +++ b/src/topology/save.c @@ -19,22 +19,43 @@   #include "tplg_local.h"   #define SAVE_ALLOC_SHIFT    (13)    /* 8192 bytes */ +#define PRINT_BUF_SIZE        (1024) +#define PRINT_BUF_SIZE_MAX    (1024 * 1024)   int tplg_save_printf(char **dst, const char *pfx, const char *fmt, ...)   {       va_list va; -    char buf[1024], *s; -    size_t n, l, t, pl; +    char *buf, *s; +    size_t n, alloc_size = PRINT_BUF_SIZE, l, t, pl; +    int ret = 0;

+    buf = malloc(alloc_size); +    if (!buf) +        return -ENOMEM;       if (pfx == NULL)           pfx = ""; +again:       va_start(va, fmt); -    n = vsnprintf(buf, sizeof(buf), fmt, va); +    n = vsnprintf(buf, alloc_size, fmt, va);       va_end(va); -    if (n >= sizeof(buf)) -        return -EOVERFLOW; +    if (n >= PRINT_BUF_SIZE_MAX) { +        ret = -EOVERFLOW; +        goto end; +    }

what this patch does is change the allocation limit from 1KB to 1MB, but the data still has no size restriction. At what point do we decide to throw an error?

If needed size is bigger than PRINT_BUF_SIZE_MAX there will be an error thrown so I don't know what should I change here. Or do you mean that private data size in binary should be restricted?