Bug 2950 - moving forward

[Thorsten Glaser]

On Tue, 7 Feb 2023, Patrick Riehecky wrote:

>I've posted the patch as a PR up at 
>https://github.com/openssh/openssh-portable/pull/346

+       char ccname[PATH_MAX] = {0};

This is a double ouch. This allocates at least 1 KiB, often
multiple times that, on the stack (big no-go) and even breaks
entirely in some circumstances (the filesystem’s is longer,
or the OS doesn’t have a fix PATH_MAX, in which case this is
a compile-time error). You’ll want pathconf(2) or, in this
case, probably asprintf(3) instead, though that’s not portable
so you’ll probably need to run it twice and allocate a buffer
in between (although you can get by with a small on-stack one
to avoid the allocation in the common small case).

Unsure if it’s safe to have create_private_runtime_directory
share one, persistent at that, directory between multiple ssh
sessions forever. Why don’t you always mkdtemp?

Should session_get_runtime_directory hardcode /tmp? Yes, the
code it replaces does that, but here’s a chance to honour
$TMPDIR, unless there’s good reasons not to.

If all callers of the new functions (session, sshpam…) to get
the runtime directory always use it in an asprintf-like or
strdup-like manner, then it might make sense to keep the value
around after first run instead of freeing it? But then, it’s
not used often enough so that’s worth that?

XDG_RUNTIME_DIR must also be validated to be an absolute path
and not used if it’s not, says the spec.

Just random thoughts from looking at this, nothing official,
//mirabilos