Pr/fix strings #567

uli42 · 2017-11-24T00:39:04Z

Fix two possible buffer overflows, one memory leak and some minor string stuff.

sunweaver · 2017-12-05T17:20:21Z

nx-X11/programs/Xserver/hw/nxagent/Display.c

@@ -1907,8 +1905,8 @@ Bool nxagentMakeIcon(Display *display, Pixmap *nxIcon, Pixmap *nxMask)
    agentIconData=nxagentIconData;
  }

-
-  snprintf(default_path, PATH_MAX-1, "/usr/NX/share/images/%s", agent_icon_name);
+  /* FIXME: use a compile tiem define here, /usr/NX is a nomachine path */


s/tiem/time/

sunweaver

I went over this quickly. Need to take a second glance once we have merged the other PRs.

uli42 · 2017-12-05T20:40:23Z

fixed typo and rebased

Ionic · 2017-12-06T05:24:24Z

nx-X11/programs/Xserver/hw/nxagent/Args.h

@@ -53,7 +53,7 @@ struct UserGeometry{
 extern Bool nxagentUseNXTrans;

 extern char nxagentSessionId[];
-extern char nxagentDisplayName[];
+extern char nxagentDisplayName[1024];


Don't do this. The size given here is merely a hint and will be ignored.

Even worse, it's allowed for the definition in a source file to deviate from that declaration.

We really should use SIZE macros for these arrays.

Args.h:

char nxagentDisplayName[]; #define NXAGENTDISPLAYNAMELENGTH 1024;

Args.c:

char nxagentDisplayName[NXAGENTDISPLAYNAMELENGTH];

An alternative way would be define size variables:

Args.h:

char nxagentDisplayName[]; size_t nxagentDisplayNameLength;

Args.c:

char nxagentDisplayName[1024]; size_t nxagentDisplayNameLength = (sizeof(nxagentDisplayName) / sizeof(*nxagentDisplayName));

The drawback is that this alternative will only work for fixed-size arrays, but not for pure pointers.

Hmm, I had to add the size here because otherwise the sizeof would fail if used in sources other than Args.c. So I am wondering if you are really right about the "only a hint" stuff.

Generally I want to get rid of all those limits as they are only artificial. So I will change the code one day to not use fixed array.

Looks like I've been wrong - T foo[]; declares an incomplete type that must be properly defined at a later point (unless the code in question does not need storage information).

Multiple declarations for he same object with different sizes lead a compile error in recent versions of both clang and gcc.

There is a special case that I confused this with: within parameters declared like this, the size is merely a hint:

b.c: In function ‘get_size_2’: b.c:9:18: warning: ‘sizeof’ on array function parameter ‘arg’ will return size of ‘char *’ [-Wsizeof-array-argument] return (sizeof (arg)); ^ b.c:8:25: note: declared here size_t get_size_2 (char arg[6]) { ^~~ (the array I pass in actually is `char foo[5]` - and that works)

For now, I'd really use a macro.

Bounding even pure pointers would also be a good idea, even if that adds an artificial limit. We can certainly raise this limit, but not doing it at all is dangerous in case the input data is invalid.

ok, I have added macros (for now). And fixed two bugs in nxagentGetDialogName: the wrong string was terminated and there where no length checks.

Ionic · 2017-12-06T06:04:11Z

nx-X11/programs/Xserver/hw/nxagent/Display.c

+        fprintf(stderr, "Error: Path too long.\n");
+        return NULL;
+      }
+
      strcpy(singlePath, path);

      breakLoop = 1;


With these changes, it doesn't make sense to keep breakLoop around. Drop it.

I don't think so. breakLoop indicates that we have processed the last path from the env variable. If we have checked that one (and found no file in that path we still need to terminate the loop).

I have improved the code:

use True/False

move variables to inner scope where possible

add comments

strip all trailing slashes, not only the last one

pass down the size of the return_path array

Ionic · 2017-12-06T06:21:42Z

nx-X11/programs/Xserver/hw/nxagent/Display.c

-
-  snprintf(default_path, PATH_MAX-1, "/usr/NX/share/images/%s", agent_icon_name);
+  /* FIXME: use a compile time define here, /usr/NX is a nomachine path */
+  snprintf(default_path, sizeof(default_path), "/usr/NX/share/images/%s", agent_icon_name);


I agree, but wonder why the original code used PATH_MAX - 1. Probably didn't know that snprintf includes the terminating 0 character in the length and only writes n - 1 elements to the target buffer (+ terminating 0). strncpy is different in this regard. They probably got confused by that...

that's what I thought, too.

Ionic · 2017-12-06T06:29:29Z

nx-X11/programs/Xserver/hw/nxagent/Font.c

@@ -1406,9 +1406,10 @@ Bool nxagentDisconnectAllFonts()

 static Bool nxagentGetFontServerPath(char * fontServerPath)
 {
-  char path[256];
+  /* ensure path is not longer that fontServerPath) */
+  char path[sizeof(fontServerPath)];


Can't do this. fontServerPath is a pure pointer. We cannot use sizeof() here.

you are right, will fix that.

Ionic · 2017-12-06T06:32:41Z

nx-X11/programs/Xserver/hw/nxagent/Keyboard.c

      FILE *keyboard_file;
-      if (keyboard_file_path == NULL)
+
+      if ((asprintf(&keyboard_file_path, "%s/keyboard", sessionpath) == -1))


Nope, we can't use asprintf yet. It's not your fault, but really mine since I didn't notice this error when reviewing some of @sunweaver's commits that used asprintf.

It doesn't mean that you have to change the code - but this PR will be blocked until I've backported os/xprintf.c so that asprintf is available across platforms.

what platforms are lacking it?

" These functions are GNU extensions, not in C or POSIX. They are also
available under *BSD. The FreeBSD implementation sets strp to NULL
on error."

By the way: backporting asprintf doesn't sound like fun: http://insanecoding.blogspot.de/2014/06/memory-management-in-c-and-auto.html

I don't know what platforms are not providing it exactly, but there certainly seem to be X.Org-supported ones that don't.

Backporting asprintf is really easy! But only because X.Org already did it. I'll take a stab at it tomorrow (or at a later time) after I went through the other open PRs.

asprintf comment obsolete.

Ionic · 2017-12-06T06:43:40Z

nx-X11/programs/Xserver/hw/nxagent/Keyboard.c

      {
-        FatalError("nxagentKeycodeConversionSetup: malloc failed.");
+        free(sessionpath);


Okay, but FatalErrror is noreturn anyway. At this point a memory leak won't hurt. Still good to free the memory.

Ionic · 2017-12-06T06:53:55Z

nx-X11/programs/Xserver/hw/nxagent/Dialog.c

@@ -308,7 +305,7 @@ void nxagentLaunchDialog(DialogType dialogType)
              DECODE_DIALOG_TYPE(dialogType), *pid, dialogDisplay);
  #endif

-  *dialogDisplay = '\0';
+  dialogDisplay[0] = '\0';


Functionally equivalent. Just a comment.

yeah, but better to read IMHO.

Ionic · 2017-12-06T06:54:54Z

nx-X11/programs/Xserver/hw/nxagent/NXdixfonts.c

@@ -112,7 +112,7 @@ static const char *_NXGetFontPath(const char *path)
     * Check the environment only once.
     */

-    if (*_NXFontPath != '\0')
+    if (_NXFontPath[0] != '\0')


Functionally equivalent. Just a comment.

Ionic · 2017-12-30T20:42:17Z

nx-X11/programs/Xserver/hw/nxagent/Font.c

-      *(fontServerPath + *path) = '\0';
+    if (len)
+    {
+      strncpy(fontServerPath, path + 1, len);


Might as well use snprintf() here.

~~will leave that for now~~ have changed it to that we now the size here; also added a MIN() guard

Ionic · 2017-12-30T21:09:42Z

nx-X11/programs/Xserver/hw/nxagent/Keyboard.c

      FILE *keyboard_file;
-      if (keyboard_file_path == NULL)
+
+      if ((asprintf(&keyboard_file_path, "%s/keyboard", sessionpath) == -1))


asprintf comment obsolete.

Ionic · 2017-12-31T01:32:06Z

nx-X11/programs/Xserver/hw/nxagent/Display.c

+        path = end + 1;
+        continue;
+      }
+
      strncpy(singlePath, path, (unsigned long)(end - path));


snprintf to drop the NULL setting.

Ionic · 2017-12-31T01:32:17Z

nx-X11/programs/Xserver/hw/nxagent/Display.c

+        fprintf(stderr, "Error: Path too long.\n");
+        return NULL;
+      }
+
      strcpy(singlePath, path);

      breakLoop = 1;


uli42 · 2018-01-02T20:07:49Z

@Ionic can you please re-check?

Ionic · 2018-01-03T05:22:38Z

nx-X11/programs/Xserver/hw/nxagent/Display.c

    }

+    /* append slash and icon name */
    if (strlen(singlePath) + strlen(iconName) + 1 < PATH_MAX)
    {
      strncat(singlePath, slash, 1);
      strcat(singlePath, iconName);


Hello, I'm Shlemiel the Painter!

How about...

snprintf(singlePath + strlen(singlePath), sizeof(singlePath), "%.1s%s", slash, iconName);

?

Ok, accepted, but the .1 is not needed because slash is only one char.

Okay, I thought thought it might be a good idea to limit reading data out of slash to one character, just in case.

Ionic · 2018-01-03T05:23:23Z

nx-X11/programs/Xserver/hw/nxagent/Display.c

    }

+    /* append slash and icon name */
    if (strlen(singlePath) + strlen(iconName) + 1 < PATH_MAX)


Replace PATH_MAX here with sizeof(singlePath)?

Can do that, but weren't you the one that didn't like that? ;-)

For global, fixed-size arrays yes. For local ones using a special macro doesn't make a lot of sense, though. PATH_MAX is a too generic one.

Ionic · 2018-01-03T05:31:49Z

nx-X11/programs/Xserver/hw/nxagent/Error.c

@@ -118,6 +118,8 @@ int nxagentErrorHandler(Display *dpy, XErrorEvent *event)



"commt" typo in commit message.

Ionic · 2018-01-03T05:42:03Z

nx-X11/programs/Xserver/hw/nxagent/Args.c

      free(nxagentKeyboard);
      nxagentKeyboard = NULL;

-      if ((size = strlen(argv[i])) < 256)


Probably good to just drop this 255 character limitation here, I haven't found a reason why this would be necessary.

Ionic · 2018-01-03T06:01:14Z

nx-X11/programs/Xserver/hw/nxagent/Args.c

    }

-    nxagentDialogName[NXAGENTDIALOGNAMELENGTH - 1] = '\0';
+    snprintf(nxagentDialogName, NXAGENTDIALOGNAMELENGTH, "NX - %.*s", length, nxagentSessionId);


I think that should be length - 1 (or 1 subtracted above) because it looks like we don't want to copy the leading - char either. At least the original code seems to have skipped it.

Ping on that?

hmm, but we are subtracting 2+MD5+1 from the length already!?

2*MD5 + 1 actually... and yes, that's fine. I got confused by the different behaviors regarding the total string length (since snprintf expects the full length, but the str* family normally the length excluding a terminating NULL character). That's not relevant here, so the code is fine. My bad.

Ionic · 2018-01-03T19:30:19Z

nx-X11/programs/Xserver/hw/nxagent/Display.c

@@ -1838,43 +1834,56 @@ static FILE *nxagentLookForIconFile(char *iconName, const char *permission,
    return NULL;
  }

-  for(breakLoop = 0; breakLoop == 0 && fptr == NULL; )
+  for (int breakLoop = False; breakLoop == False && fptr == NULL; )


Can drop breakLoop by using:

for (char *end = path; end == NULL && fptr == NULL; )

No, I'd have to check for end != NULL && fprt == NULL. Done.

Yes, sorry.

Ionic · 2018-01-03T19:41:07Z

nx-X11/programs/Xserver/hw/nxagent/Keyboard.c

      if ((keyboard_file = fopen(keyboard_file_path, "w"))) {
-        if (drules)
+        if (drules != NULL)


Changing this makes it weirdly inconsistent with the later code?

Ping on that?

uli42 · 2018-01-03T21:40:21Z

ok, I fixed the four things you mentioned

…ffer lengths

…appropriate

Fix write past the end of singlePath if PATH contains dirs longer than PATH_MAX.

Also comment the code and convert error messages to warnings.

and insert one single space

This is a partial copy from XlibInt.c's _XPrintDefaultError, which had some minor changes since being copied. Update to reflect these changes. Also comment some more lines as their result was unused an cluttered the output.

I am not sure about the maximum font name length in X but just in case use snprintf instead of memcpy to be sure nothing dangerous can happen here.

even if we issue a FatalError afterwards

Attributes GH PR #567: #567

uli42 requested a review from Ionic November 24, 2017 00:39

sunweaver reviewed Dec 5, 2017

View reviewed changes

uli42 force-pushed the pr/fix_strings branch 2 times, most recently from 993fcad to b9d1443 Compare December 5, 2017 20:40

Ionic requested changes Dec 6, 2017

View reviewed changes

uli42 force-pushed the pr/fix_strings branch 2 times, most recently from 80a4625 to e5c9a65 Compare December 11, 2017 22:09

Ionic requested changes Dec 31, 2017

View reviewed changes

uli42 force-pushed the pr/fix_strings branch 2 times, most recently from d971931 to 1a36181 Compare January 2, 2018 20:03

uli42 force-pushed the pr/fix_strings branch 7 times, most recently from c60fcc3 to c96c7c0 Compare January 3, 2018 01:35

Ionic requested changes Jan 3, 2018

View reviewed changes

uli42 force-pushed the pr/fix_strings branch 2 times, most recently from d1da4bd to a711df7 Compare January 3, 2018 21:38

uli42 force-pushed the pr/fix_strings branch from a711df7 to 0e0fa50 Compare January 6, 2018 13:16

Ionic approved these changes Jan 7, 2018

View reviewed changes

uli42 added 4 commits January 7, 2018 01:27

Dialog.c,Display.c,Font.c,NXdixfonts.c: don't use hardcoded string bu…

5987a7b

…ffer lengths

Dialog.c,Display.c,NXdixfonts.c: replace strncpy() by snprintf where …

4a34578

…appropriate

Dialog.c: fix possible buffer overflows

3de6bc7

Fix write past the end of singlePath if PATH contains dirs longer than PATH_MAX.

Keyboard.c: fix memory leak

0e2c4df

uli42 added 24 commits January 7, 2018 01:27

Keyboard.c: improve user messages for keyboard file handling

08912ef

Keyboard.c: replace malloc + strcpy by asprintf

2836c72

Font.c: shorten string handling

9d5c83e

Screen.c: string handling improvements

677da7f

Font.c: make nxagentGetFontServerPath more readable

9a6b90c

Replace hardcoded string lengths by macros

ea1e0be

Display.c: pass down buffer size to nxagentLookForIconFile

19a3918

Also comment the code and convert error messages to warnings.

Font.c: pass down size

a8a6938

Error.c: remove empty ifdef

6c080c8

Error.c: remove trailing whitespace

654422a

and insert one single space

Error.c: use standard file descriptor macros

7d87e5a

Error.c: replace strcpy/strcat by snprintf

525e151

Error.c: update nxagentPrintError

96e4300

This is a partial copy from XlibInt.c's _XPrintDefaultError, which had some minor changes since being copied. Update to reflect these changes. Also comment some more lines as their result was unused an cluttered the output.

Args.c: use strdup instead of malloc

dc61b6c

Args.c, Error.[ch]: introduce own length macro for nxagentClientsLogName

74c21e9

Args.c: simplify nxagentGetDialogName()

cac1af5

Font.c: replace memcpy by sprintf preventing possible buffer overflows

56fa234

I am not sure about the maximum font name length in X but just in case use snprintf instead of memcpy to be sure nothing dangerous can happen here.

Font.c: free possibly allocated mem

054ae84

even if we issue a FatalError afterwards

Font.c: replace malloc + strcpy by strdup + fix memleak

1854700

Keystroke.c: replace calloc + 2 * strcpy by asprintf

a0e985f

Keyboard.c: rework string allocation/building

11b2b06

Events.c: add FIXME

27a31d0

Error.c: replace malloc+strcpy by strdup

b89b809

Display.c: drop helper variable in loop

23c36c2

Ionic force-pushed the pr/fix_strings branch from 0e0fa50 to 23c36c2 Compare January 7, 2018 00:28

Ionic merged commit 23c36c2 into ArcticaProject:3.6.x Jan 7, 2018

Ionic added a commit that referenced this pull request Jan 7, 2018

Merge branch 'uli42-pr/fix_strings' into 3.6.x

5f9f744

Attributes GH PR #567: #567

		@@ -118,6 +118,8 @@ int nxagentErrorHandler(Display dpy, XErrorEvent event)

Pr/fix strings #567

Pr/fix strings #567

Conversation

uli42 commented Nov 24, 2017

Choose a reason for hiding this comment

sunweaver left a comment

Choose a reason for hiding this comment

uli42 commented Dec 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uli42 Jan 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uli42 commented Jan 2, 2018

Choose a reason for hiding this comment

uli42 Jan 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uli42 Jan 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uli42 commented Jan 3, 2018

uli42 Jan 2, 2018 •

edited

uli42 Jan 3, 2018 •

edited

uli42 Jan 6, 2018 •

edited