[PATCH i-g-t] lib/igt_debugfs: Prevent compiler warning on unchecked printf format

[PATCH i-g-t] lib/igt_debugfs: Prevent compiler warning on unchecked printf format

[Gabriel Krisman Bertazi]

Commit 34a54192e1fb ("lib/igt_debugfs: Add extended helper to format
crc to string") introduced the following compiler warning:

  igt_debugfs.c: In function ‘igt_crc_to_string_extended’:
  igt_debugfs.c:373:4: warning: format not a string literal, argument
  types not checked [-Wformat-nonliteral]
      i == (crc->n_words - 1) ? '\0' : delimiter);

This patch addresses the warning while also preventing a possible bad
memory access access if n_words is > 64.  I have no clue why we
care about the padding size (crc_size), but since this was added
recently, I'd rather leave it alone.

Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.co.uk>
---
 lib/igt_debugfs.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
index 2aa335866066..ee1f0f544824 100644
--- a/lib/igt_debugfs.c
+++ b/lib/igt_debugfs.c
@@ -351,7 +351,7 @@ bool igt_check_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
  * igt_crc_to_string_extended:
  * @crc: pipe CRC value to print
  * @delimiter: The delimiter to use between crc words
- * @crc_size: the number of bytes to print per crc word (either 4 or 2)
+ * @crc_size: the number of bytes to print per crc word (between 1 and 4)
  *
  * This function allocates a string and formats @crc into it, depending on
  * @delimiter and @crc_size.
@@ -362,16 +362,19 @@ bool igt_check_crc_equal(const igt_crc_t *a, const igt_crc_t *b)
 char *igt_crc_to_string_extended(igt_crc_t *crc, char delimiter, int crc_size)
 {
 	int i;
-	char *buf = calloc(128, sizeof(char));
-	const char *format[2] = { "%08x%c", "%04x%c" };
+	int len = 0;
+	int field_width = 2 * crc_size; /* Two chars per byte. */
+	char *buf = malloc((field_width+1) * crc->n_words * sizeof(char));
 
 	if (!buf)
 		return NULL;
 
 	for (i = 0; i < crc->n_words; i++)
-		sprintf(buf + strlen(buf), format[crc_size == 2], crc->crc[i],
-			i == (crc->n_words - 1) ? '\0' : delimiter);
+		len += sprintf(buf + len, "%0*x%c", field_width,
+			       crc->crc[i], delimiter);
 
+	/* Eat the last delimiter */
+	buf[len - 1] = '\0';
 	return buf;
 }
 
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] lib/igt_debugfs: Prevent compiler warning on unchecked printf format

[Arkadiusz Hiler]

On Thu, Jul 27, 2017 at 01:51:28AM -0300, Gabriel Krisman Bertazi wrote:
> Commit 34a54192e1fb ("lib/igt_debugfs: Add extended helper to format
> crc to string") introduced the following compiler warning:
> 
>   igt_debugfs.c: In function ‘igt_crc_to_string_extended’:
>   igt_debugfs.c:373:4: warning: format not a string literal, argument
>   types not checked [-Wformat-nonliteral]
>       i == (crc->n_words - 1) ? '\0' : delimiter);
> 
> This patch addresses the warning while also preventing a possible bad
> memory access access if n_words is > 64.  I have no clue why we
> care about the padding size (crc_size), but since this was added
> recently, I'd rather leave it alone.
> 
> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.co.uk>
Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com>

and pushed, thanks!

-- 
Cheers,
Arek
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] lib/igt_debugfs: Prevent compiler warning on unchecked printf format

[Paul Kocialkowski]

On Thu, 2017-07-27 at 01:51 -0300, Gabriel Krisman Bertazi wrote:
> Commit 34a54192e1fb ("lib/igt_debugfs: Add extended helper to format
> crc to string") introduced the following compiler warning:
> 
>   igt_debugfs.c: In function ‘igt_crc_to_string_extended’:
>   igt_debugfs.c:373:4: warning: format not a string literal, argument
>   types not checked [-Wformat-nonliteral]
>       i == (crc->n_words - 1) ? '\0' : delimiter);

Thanks for the fix, I wasn't even aware that the printf format you used
("%0*x%c") existed! This is definitely what I should have done. And it's
definitely better to get rid of the fixed-length allocated buffer.

> This patch addresses the warning while also preventing a possible bad
> memory access access if n_words is > 64.  I have no clue why we
> care about the padding size (crc_size), but since this was added
> recently, I'd rather leave it alone.

That's because the Chamelium CRCs are actually 2 bytes, not the full 4
that the type allows, so this avoids printing heading zeros all the
time.

> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.co.uk>
> ---
>  lib/igt_debugfs.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> index 2aa335866066..ee1f0f544824 100644
> --- a/lib/igt_debugfs.c
> +++ b/lib/igt_debugfs.c
> @@ -351,7 +351,7 @@ bool igt_check_crc_equal(const igt_crc_t *a, const
> igt_crc_t *b)
>   * igt_crc_to_string_extended:
>   * @crc: pipe CRC value to print
>   * @delimiter: The delimiter to use between crc words
> - * @crc_size: the number of bytes to print per crc word (either 4 or
> 2)
> + * @crc_size: the number of bytes to print per crc word (between 1
> and 4)
>   *
>   * This function allocates a string and formats @crc into it,
> depending on
>   * @delimiter and @crc_size.
> @@ -362,16 +362,19 @@ bool igt_check_crc_equal(const igt_crc_t *a,
> const igt_crc_t *b)
>  char *igt_crc_to_string_extended(igt_crc_t *crc, char delimiter, int
> crc_size)
>  {
>  	int i;
> -	char *buf = calloc(128, sizeof(char));
> -	const char *format[2] = { "%08x%c", "%04x%c" };
> +	int len = 0;
> +	int field_width = 2 * crc_size; /* Two chars per byte. */
> +	char *buf = malloc((field_width+1) * crc->n_words *
> sizeof(char));
>  
>  	if (!buf)
>  		return NULL;
>  
>  	for (i = 0; i < crc->n_words; i++)
> -		sprintf(buf + strlen(buf), format[crc_size == 2],
> crc->crc[i],
> -			i == (crc->n_words - 1) ? '\0' : delimiter);
> +		len += sprintf(buf + len, "%0*x%c", field_width,
> +			       crc->crc[i], delimiter);
>  
> +	/* Eat the last delimiter */
> +	buf[len - 1] = '\0';
>  	return buf;
>  }
>  
-- 
Paul Kocialkowski <paul.kocialkowski@linux.intel.com>
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx