Re: [Iup-users] Fix for GCC 10.3 warning (buffer overflow, undefined behaviour) in cd/src/cd_text.c (cd-r897)

2021-09-05 Thread Antonio Scuri
  Patch applied and committed to the SVN.

Thanks,
Scuri


Em sáb., 4 de set. de 2021 às 21:15, sur-behoffski <
sur_behoff...@grouse.com.au> escreveu:

> G'day,
>
> I've decided to focus exclusively on fixing "undefined behaviour" --
> specifically, potential buffer overflows -- found by GCC 10.3, in IM, CD
> and IUP.  I'm using only the the "-Wformat-overflow=" warnings.
>
> I found no cases in IM-r820, so am now looking at CD-r897.
>
> Typically, in a reasonable program, the boundaries would not be pushed...
> but I'm trying to close off all such loopholes, as a defect in one area
> may invalidate assumptions in another area.  Therefore, these changes
> can limit collateral damage. Such damage could otherwise potentially
> open up space for malware/hackers.
>
> I'm trying VERY hard to make the changes as non-intrusive as possible.
>
> This first case, and, I suspect most cases, will be using "snprintf(3)"
> instead of "sprintf(3)" or perhaps "strcat(3)".
>
> The patch can increase the cases where the function returns a NULL
> pointer (if snprintf returns -1, or if it signals that a buffer overflow
> has been thwarted).
>
> In addition, copying canvas->native_font to private static buffer
> native_font is deferred until after the CD_QUERY case.  Apart from any
> third-party timing/race conditions, the earlier copy is pointless in the
> CD_QUERY case, as the buffer is overwritten in order to return the query
> (this behaviour is true of both the original and the modified code).
>
> A SVN patch is attached.
>
> 
>
> The GCC warning:
>
>  directive writing 1 byte into a region of size between 0 and
>  [-Wformat-overflow=]:
> cd_text.c:310:[Function:cdCanvasNativeFont]:   ' '  1023
>
> 
>
> Current (cd-r897) partial function listing, cdCanvasNativeFont():
>
>
>  char* cdCanvasNativeFont(cdCanvas* canvas, const char* font)
> {
>   static char native_font[1024] = "";
>
>   assert(canvas);
>   if (!_cdCheckCanvas(canvas)) return NULL;
>
>   strcpy(native_font, canvas->native_font);
>
>   if (font == (char*)CD_QUERY)
>   {
> char style[200] = " ";
> if (canvas->font_style&CD_BOLD)
>   strcat(style, "Bold ");
> if (canvas->font_style&CD_ITALIC)
>   strcat(style, "Italic ");
> if (canvas->font_style&CD_UNDERLINE)
>   strcat(style, "Underline ");
> if (canvas->font_style&CD_STRIKEOUT)
>   strcat(style, "Strikeout ");
>
> sprintf(native_font, "%s,%s %d", canvas->font_type_face, style,
> canvas->font_size);
> return native_font;
>   }
>
>   /* [...] remainder elided (not changed) */
>
>
> 
>
> Proposed, cdCanvasNativeFont():
>
>
> char* cdCanvasNativeFont(cdCanvas* canvas, const char* font)
> {
>   static char native_font[1024] = "";
>
>   assert(canvas);
>   if (!_cdCheckCanvas(canvas)) return NULL;
>
>   if (font == (char*)CD_QUERY)
>   {
> int result;
>
> result = snprintf(native_font, sizeof(native_font),
>   "%s,%s%s%s%s %d",
>   canvas->font_type_face,
>   (canvas->font_style&CD_BOLD)  ? " Bold"  :
> "",
>   (canvas->font_style&CD_ITALIC)? " Italic":
> "",
>   (canvas->font_style&CD_UNDERLINE) ? " Underline" :
> "",
>   (canvas->font_style&CD_STRIKEOUT) ? " Strikeout" :
> "",
>   canvas->font_size);
> if ((result < 0) || (result >= sizeof(native_font)))
>   return NULL;
> return native_font;
>   }
>
>   strcpy(native_font, canvas->native_font);
>
>   /* [...] remainder elided (not changed) */
>
>
> 
>
> ___
> Iup-users mailing list
> Iup-users@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/iup-users
>
___
Iup-users mailing list
Iup-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/iup-users


[Iup-users] Fix for GCC 10.3 warning (buffer overflow, undefined behaviour) in cd/src/cd_text.c (cd-r897)

2021-09-04 Thread sur-behoffski
G'day,

I've decided to focus exclusively on fixing "undefined behaviour" --
specifically, potential buffer overflows -- found by GCC 10.3, in IM, CD
and IUP.  I'm using only the the "-Wformat-overflow=" warnings.

I found no cases in IM-r820, so am now looking at CD-r897.

Typically, in a reasonable program, the boundaries would not be pushed...
but I'm trying to close off all such loopholes, as a defect in one area
may invalidate assumptions in another area.  Therefore, these changes
can limit collateral damage. Such damage could otherwise potentially
open up space for malware/hackers.

I'm trying VERY hard to make the changes as non-intrusive as possible.

This first case, and, I suspect most cases, will be using "snprintf(3)"
instead of "sprintf(3)" or perhaps "strcat(3)".

The patch can increase the cases where the function returns a NULL
pointer (if snprintf returns -1, or if it signals that a buffer overflow
has been thwarted).

In addition, copying canvas->native_font to private static buffer
native_font is deferred until after the CD_QUERY case.  Apart from any
third-party timing/race conditions, the earlier copy is pointless in the
CD_QUERY case, as the buffer is overwritten in order to return the query
(this behaviour is true of both the original and the modified code).

A SVN patch is attached.



The GCC warning:

 directive writing 1 byte into a region of size between 0 and 
 [-Wformat-overflow=]:
cd_text.c:310:[Function:cdCanvasNativeFont]:   ' '  1023



Current (cd-r897) partial function listing, cdCanvasNativeFont():


 char* cdCanvasNativeFont(cdCanvas* canvas, const char* font)
{
  static char native_font[1024] = "";

  assert(canvas);
  if (!_cdCheckCanvas(canvas)) return NULL;

  strcpy(native_font, canvas->native_font);

  if (font == (char*)CD_QUERY)
  {
char style[200] = " ";
if (canvas->font_style&CD_BOLD)
  strcat(style, "Bold ");
if (canvas->font_style&CD_ITALIC)
  strcat(style, "Italic ");
if (canvas->font_style&CD_UNDERLINE)
  strcat(style, "Underline ");
if (canvas->font_style&CD_STRIKEOUT)
  strcat(style, "Strikeout ");

sprintf(native_font, "%s,%s %d", canvas->font_type_face, style, 
canvas->font_size);
return native_font;
  }

  /* [...] remainder elided (not changed) */




Proposed, cdCanvasNativeFont():


char* cdCanvasNativeFont(cdCanvas* canvas, const char* font)
{
  static char native_font[1024] = "";

  assert(canvas);
  if (!_cdCheckCanvas(canvas)) return NULL;

  if (font == (char*)CD_QUERY)
  {
int result;

result = snprintf(native_font, sizeof(native_font),
  "%s,%s%s%s%s %d",
  canvas->font_type_face,
  (canvas->font_style&CD_BOLD)  ? " Bold"  : "",
  (canvas->font_style&CD_ITALIC)? " Italic": "",
  (canvas->font_style&CD_UNDERLINE) ? " Underline" : "",
  (canvas->font_style&CD_STRIKEOUT) ? " Strikeout" : "",
  canvas->font_size);
if ((result < 0) || (result >= sizeof(native_font)))
  return NULL;
return native_font;
  }

  strcpy(native_font, canvas->native_font);

  /* [...] remainder elided (not changed) */




Index: cd/src/cd_text.c
===
--- cd/src/cd_text.c	(revision 897)
+++ cd/src/cd_text.c	(working copy)
@@ -293,24 +293,25 @@
   assert(canvas);
   if (!_cdCheckCanvas(canvas)) return NULL;
 
-  strcpy(native_font, canvas->native_font);
-
   if (font == (char*)CD_QUERY)
   {
-char style[200] = " ";
-if (canvas->font_style&CD_BOLD)
-  strcat(style, "Bold ");
-if (canvas->font_style&CD_ITALIC)
-  strcat(style, "Italic ");
-if (canvas->font_style&CD_UNDERLINE)
-  strcat(style, "Underline ");
-if (canvas->font_style&CD_STRIKEOUT)
-  strcat(style, "Strikeout ");
+int result;
 
-sprintf(native_font, "%s,%s %d", canvas->font_type_face, style, canvas->font_size);
+result = snprintf(native_font, sizeof(native_font),
+  "%s,%s%s%s%s %d",
+  canvas->font_type_face,
+  (canvas->font_style&CD_BOLD)  ? " Bold"  : "",
+  (canvas->font_style&CD_ITALIC)? " Italic": "",
+  (canvas->font_style&CD_UNDERLINE) ? " Underline" : "",
+  (canvas->font_style&CD_STRIKEOUT) ? " Strikeout" : "",
+  canvas->font_size);
+if ((result < 0) || (result >= sizeof(native_font)))
+  return NULL;
 return native_font;
   }
 
+  strcpy(native_font, canvas->native_font);
+
   if (!font || font[0] == 0)
 return native_font;
 
___
Iup-users mailing list
Iup-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/iup-users