[gambler]

Is this the official sudo repository?

https://www.sudo.ws/repos/sudo/file/f75f786eddd5

It has more than 10 thousand commits, ~600 files, and close to 11MB of C code. Also, the code seems to have no unit tests, the main file is 1.4K lines long, has quintuple-nested conditionals and liberally uses goto statements.

Am I missing something here?

[notacoward]

Welcome to the world of code older than most people on HN. Sudo is about 30 years old. A lot of code from that era, including the kernels and utilities for most flavors of UNIX, was written in a similar style. The "goto done" idiom was a pretty common form of error handling in Plain Old C, and was more readable/verifiable than some alternatives such as deeply nested functions. Remember, this is C - no objects (so no destructors for RAII), no exceptions, etc. The lack of unit tests is explainable by the fact that tests were often external to the running code and considered proprietary. It's all too easy for people who never had to deal with serious portability issues or even know what threats existed in the tty-handling stack to hold code like this up against a standard that was still decades in the future and find it wanting. Is it useful?

[jblow]

Some of the points you raise are matters of style (I have files that are 10k lines long and think they are better that way; also I don't think unit tests are as useful as claimed), but yeah, when you have 600 files and 11MB of code to do something that is supposed to be simple and also is security-critical ... you have a big problem.

[gambler]

What a coincidence. I watched your video on Preventing the Collapse of Civilization just yesterday. (Mostly agree with things said there, with the exception of the part about Smalltalk. Ironically, the video was recommended to me in a Smalltalk chat channel.) Seems relevant.

>Some of the points you raise are matters of style (I have files that are 10k lines long and think they are better that way

It's true that line counts are a matter of style. However, some coding styles tax cognitive bandwidth and immediate memory capacity more than others. This causes people to skim over specifics and miss bugs. Even if you are comfortable with a 10K LOC file you wrote, whomever reviews your code will probably be overwhelmed until they fully comprehend its structure. Grouping related things into smaller files is a way to focus their attention and communicate intended relations between code units.

> also I don't think unit tests are as useful as claimed

In general, I agree. Not a fan of TDD zealotry. But in this particular case some unit tests would be beneficial. There is a lot of stuff going in some methods.

They do seem to run PVS-Studio static analysis, which is somewhat reassuring, but I don't think that's enough in code that is so important, complex and widely used.

[jblow]

It should be very obvious once you think about it that breaking code into more files, and procedures into subprocedures, is pushing complexity around rather than simplifying ... and it’s pushing the complexity from somewhere visible, into an invisible structure that the viewer has to then reconstruct in his mind. I think in a great many cases it’s not a good idea.

[yjftsjthsd-h]

There is a reason the OpenBSD folks decided to write doas:)

[shawnz]

> I have files that are 10k lines long and think they are better that way

Whether you prefer this practice stylistically or not doesn't change the fact that it is objectively worse in terms of code quality and maintainability, even if you are able to compensate for those deficits with your own skill/experience.

You might have no problem successfully navigating that code for which you've already built a complete mental model around, but any future collaborators or inheritors of that code will have to waste hours or days rebuilding that mental model which you could have just codified in the structure of the source code.

[inimino]

> objectively worse

That's an opinion. There's nothing objective about the assertion that many small files are inherently more secure. However, that many small files and one large file have equivalent power, can express all the same programs, and can be equally secure or insecure... those actually are all objective facts.

[shawnz]

I didn't say it was objectively more secure. Obviously it's possible to have code that is of poor quality/maintainability but still happens to be more secure than its alternatives. Maintaining good code quality is not a magic bullet that prevents all bugs.

[inimino]

Then replace secure with "maintainable" or "high quality". It's still a subjective opinion, unless you can prove that large files are inherently less maintainable or lower quality (which you can't, because all the arguments here are subjective and, like almost everything in software development, these opinions are driven by fashion rather than any evidence).

It's popular to use "objectively" for "clearly" and I've been guilty of this myself, but let's try to reserve the term for actual matters of fact, lest it lose any meaning.

[notacoward]

Besides being unclear whether one is better or worse, it's not clear there's even a practical difference when a significant number of programmers will be navigating and learning code through an IDE anyway. They'll be jumping from function to function, barely even aware of which file each one is in.

Personally I prefer smaller files, but that's only because of compile times. I started my career with punch cards, so I know what slow compile times look like. For a long time they seemed to be getting ever shorter, until I switched to a "modern C++" project. Now compile times are not quite back to the punch-card days, but back to maybe late 90s, and breaking stuff up into smaller files helps.

[nullc]

There is a BIG difference between "always moves to the end of the function for error handling" use of goto, and general unstructured control flow goto.

The former isn't significantly different than exceptions, except that it's probably less likely than exceptions to cause unexpected errors because its more explicit.

At a very quick glance that's what the goto usage looks like in the codebase. This isn't to say that I think it's all appropriate, but if it was, I don't think your comment would have been any different.

There are a lot of really hot opinions on coding style and various fashions, but very little of it is backed up with research that connects it to defect rate.

I agree it would be good if sudo had an internal test suite (particularly system tests and fuzzing harnesses)-- but you also shouldn't discount the large amount of testing that goes on against it externally through both usage and external efforts.

[wbkang]

No, it is not very good. I remember this one particular example that I was surprised to see years ago, and I actually encountered the bug at work. There is a constant named MAX_UID_T_LEN to denote the maximum number of characters that a UID can have. For whatever reason, LDAP-synced UIDs at work had many digits and sudo would fail to work because the code only worked UIDs that have MAX_UID_T_LEN - 1 characters.

The bug was fixed in 2012 - https://www.sudo.ws/changes.html: Use MAX_UID_T_LEN + 1 for uid/gid buffers, not MAX_UID_T_LEN to prevent potential truncation. Bug #562.

[ape4]

Hopefully a there's a clear separation between parsing the text files and a small section that allows or disallows.