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 _______________________________________________ openssh-unix-dev mailing list openssh-unix-dev@xxxxxxxxxxx https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
