Code in patched Cygwin for calculating path to command interpreter

Question

I'm working on a fork of Cygwin for native Windows development.

In this commit, I'm trying to improve the security of code that spawns cmd.exe, used by functions like system, popen.

What I would like to focus on is this:

static const char *cmd_exe_path = NULL;

static void init_cmd_exe_path(void)
{
  char sysdir[NT_MAX_PATH];
  char cmdname[] = "cmd.exe";
  unsigned int nchars;

  if ((nchars = GetSystemDirectoryA(sysdir, sizeof sysdir)) < sizeof sysdir) {
    unsigned int total = nchars + 1 + sizeof cmdname + 1;
    char *path = static_cast<char *>(cmalloc_abort(HEAP_STR, total));
    snprintf(path, total, "%s\\%s", sysdir, cmdname);
    cmd_exe_path = path;
  }
}

const char *get_cmd_exe_path()
{
  static pthread_once_t cmd_exe_once = PTHREAD_ONCE_INIT;
  pthread_once (&cmd_exe_once, init_cmd_exe_path);
  return cmd_exe_path;
}

The idea here is to avoid relying on a hard-coded path like C:\Windows\System32\cmd.exe, as well as to avoid the COMSPEC environment variable which could be commandeered by an attacker. Comment about anything are welcome: API usage, bugs, security concerns, correct use of Cygwin internals (I have no idea whether HEAP_STR is the best choice in cmalloc_abort).

Oops, I see I used (void), out of habit, on the static function, even though this is C++.

Answer 1

You're allocating too much memory:

unsigned int total = nchars + 1 + sizeof cmdname + 1;

You only need 1 terminating null at the end of the string. This is already accounted for in the size of cmdname (since you declare it as a string initialized []). You can simply allocate:

unsigned int total = nchars + 1 + sizeof cmdname;

Or, if you add the "\" onto cmdname you could do:

char cmdname[] = "\\cmd.exe";
unsigned int total = nchars + sizeof cmdname;

Answer 2

This belongs in a comment (which I cannot make) but why are you declaring cmd_exe_path as const? And why is cmdname hard-declared as an array? It may improve readability if it was a char*.