LWN: Comments on "OpenSSH 9.2 released"

OpenSSH 9.2 released

mrugiero — Thu, 16 Feb 2023 17:18:33 +0000

From the mention to `goto` I infer OP means not using the `return` keyword directly but jumping to a label at the end of the function that does cleanup and then returns, rather than not exiting the function early on errors. It's a common pattern, I've seen it called "goto exit" and "goto cleanup".

OpenSSH 9.2 released

nix — Sun, 12 Feb 2023 12:07:40 +0000

Ban use of early return, thus making early error checks much less readable, discouraging them, for the sake of edge cases like portability to weird embedded C compilers?

Aren't you optimizing for entirely the wrong thing here?

OpenSSH 9.2 released

smoogen — Tue, 07 Feb 2023 13:27:39 +0000

Most likely because that is how they have written code for 30 to 40 years and so 'know' it better than some other style. That means it has become the dialect they are most familiar with and able to usually catch various errors better in it than others. Changing the code dialect usually requires a lot of retraining and usually adds a lot of errors. Those errors can end up being exploitable which then re-enforces not changing the style.

Changes in coding dialect usually require an outside force to bring in the changes and show how they work.

OpenSSH 9.2 released

paulj — Tue, 07 Feb 2023 09:58:03 +0000

For this reason, I like to have modules in C code of mine that allocate objects, have a foo_free(foo **obj) function. The foo module's free function can then NULL the pointer in the caller.

Far from perfect, the caller or other code may have stashed other pointers, but at least it ensures 'ownership' at the call site is cleanly transferred and destroyed.

More complete solutions would involve providing functions to handle transfer of assignment, rather than using C =, and/or weak pointer/reference abstraction. Whether that's worth it depends on the complexity of the lifetime management of whatever objects you're dealing with. (And many will say "Why not Rust these days instead?").

OpenSSH 9.2 released

ibukanov — Fri, 03 Feb 2023 18:11:46 +0000

Personally I do not like auto ptr and similar macros for C. I rather prefer explicit free at the end of a function with a static checker to make sure that it is always called. At least it makes porting code to weird embedded C compiler much less problematic.

As for rarely used cleanup paths a rather effective strategy IMO is to ban use of early return in C and always use goto instead to jump to the common cleanup code shared between success and error paths.

OpenSSH 9.2 released

mss — Fri, 03 Feb 2023 17:39:48 +0000

free() and similar destructor-type functions in the middle of the function with no comments and no assignments of the pointer to NULL after that.

+1 for that, there's no reason not to set the object pointer to NULL after freeing the pointed-to object.
This turns a possible re-use into a NULL pointer dereference, which is almost always "just" a DoS risk.

For example, Glib has a nice g_clear_pointer () macro for doing exactly that, which makes it easy to audit the code that it does this clearing everywhere - just by searching for freestanding free () calls.

However, this does not replace clear object ownership rules and annotations.

For function-local objects or for holding a temporary object reference in a code block RAII-like self-freeing constructs as Glib's g_autoptr(TypeName) are very useful.
These especially make error cleanups much simpler (and these paths are often not well tested and bit-rot quickly).

AFAIK, Linux kernel would benefit from these constructs, too.

OpenSSH 9.2 released

ibukanov — Fri, 03 Feb 2023 01:17:11 +0000

My complain is not about tabs or other cosmetics. It is about:

Cryptic names of variables
Unclear ownership of memory not reflected in comments.
free() and similar destructor-type functions in the middle of the function with no comments and no assignments of the pointer to NULL after that.
Reuse of pointers to point to different memory-allocated things instead of introducing a new name for the new pointer.
Attempts to save vertical space via combing memory-allocating assignments and the if check.

And the rules are violated in a single function in new code written in C in a system-critical service...

OpenSSH 9.2 released

wahern — Fri, 03 Feb 2023 00:27:38 +0000

The code *style* is traditional BSD KNF (Kernel Normal Form). Like most older styles in the Unix world it uses hard tabs. And unlike more recent derivatives it still uses tabs for alignment (not just indentation), including columnar alignment of types and type names. So the code itself will look messier unless your tab stop is set to 8.

But if the *architecture* of OpenSSH seems messy, that's because it is messy. It's a fork of the *original* SSH implementation by Tatu Ylönen, and like any such original piece of software which coevolved with the functional concepts, the awkwardness is evident (and challenging). The codebase has seen tremendous changes, but the basic program layout and processing architecture, including the peculiar event loop, data model, etc, remain. That fundamental flavor of the codebase could never really be extirpated without effectively doing a complete rewrite. Newer components, which I think account for the majority of SLoC, still necessarily reflect the design (such as it is) of the original codebase.

Sudo has similar problems for similar reasons--original software that created its niche--though sudo is 10-15 years older and corporate demands and funding resulted in sudo growing much more complex than was probably reasonable. That's why OpenBSD switched to doas, with the blessing and assistance of the long-time sudo maintainer.

OpenSSH 9.2 released

ibukanov — Thu, 02 Feb 2023 23:09:06 +0000

The relevant code is dense and cryptic. I understand why OpenSSH will continue to use the standard C, not a memory safe language or at least a safe subset of C with safety enforced via tooling. But how do the authors justify to write in such code style on top of plain C?