Ars Technica article focused on Wireguard regarding FreeBSD

mark_j

Daemon

Reaction score: 681
Messages: 1,192

No. Writing tests/constraints that unconditionally return true are not among the skills that I would consider the most wanted for a kernel

I know, seriously? This is just horrible in every way. I note some people raised concerns about all the #ifdefs in there, yet none said, "hey, pull this code until it can be reviewed/cleaned up."
I guess this is where someone like Linus would do exactly that.

I don't know the politics of this but can another core member "revoke" code for review? If they can't, they should.

I must add, I don't see this as a catastrophe UNLESS they (core) do not learn from this.
 

mark_j

Daemon

Reaction score: 681
Messages: 1,192

You are agreeing that I am right and that is true--nothing happened. Thank you. How it got to that point is an issue that needs addressing as I have stated more than once. Going on and on about something that did not happen is pointless and time consuming for us and this thread that is only a concern for forums and Ars but nowhere else that matters.

But by luck rather than good management. If you think luck is a way to control a software project, then, well good luck running it, you'll need it.

This is a horrible question to ask and I've seen it asked on the amateur forums and Ars, too. Every project everywhere has something that was missed and had close calls where one can ask that same question. Again, it's pointless to ask and only worrisome to those who only want to make themselves feel superior.

Well at the risk of seeming superior this is pure nonsense. Luck has no place in a software system. Why test at all, if luck will determine your outcome. Let's just wait and see? Maybe someone will find my bugs, maybe they won't or maybe my code is bug free? Hey, I'm feeling lucky today, let's place 40,000 lines of unaudited code in the kernel and cross our fingers. We have a saying here: "She'll be right, mate!".

You seem unwilling to believe the HUGE difference between missing poor code through mistakes and poor code through negligence. This is the latter. If you can't see the difference, then I cannot explain it any more clearer it seems.
 

Mjölnir

Daemon

Reaction score: 1,502
Messages: 2,114

Please compare the following two occurrences:
  • When I filed in a (truly minor) bug report recently (PR 253861), I was kindly asked to write a test to asure it could not happen again (because the genuine author is too busy commiting more flaws like that?). So, to proceed with an already existing minor bug, the reporter is requested to provide tests... versus:
  • FMLU that guy from the WireGuard project (Jason A. Donenfeld) wrote a bunch of tests for the FreeBSD in-kernel implementation, when several dozens kLoC have already been commited... So, to commit dozens of kLoC that conforms to numerous commonly accepted worst-practices of C programming, all you need is mentorship from Sean Bruno. Just let the others write the tests.
Farewell & bon voyage, almighty free BeaSD, and rest in peace.
 

drhowarddrfine

Son of Beastie

Reaction score: 2,284
Messages: 4,264

But by luck rather than good management. If you think luck is a way to control a software project, then, well good luck running it, you'll need it.
Which has nothing to do with what I said. The system works. It works well. In this case, a hole was found--a bug in the system. The bug will be fixed but the software was never released. In the meantime, nothing happened.

You are now stating FreeBSD's code, due to this one issue, is managed by luck which is a preposterous statement.
 

troublemaker

Member

Reaction score: 5
Messages: 43

From the Wiki: Phabricator is a FreeBSD-hosted service that provides pre-commit code review workflows.

A website where a developer proposes a change or addition to FreeBSD with code changes on display.
There are commiters who are summoned by 'Herald' the phab bot for code review and some commiters will set themselves as a reviewer.
So the code review should provide fixes for code style and function.
The code author has to fix code to pass go at each stoppage..
Then when enough reviewers approve it the code gets commited to HEAD.

So phab is a code fixup and review process.
Our code collabration tool.
Ah thanks for that.
So putting the committer as reviewer doesn't look very helpful, but from what you write I understand there are more reviewers.
And that is the normal process? So basically I can assume that the code that goes to HEAD is reviewed (not by the committer), and therefore the problem in this case is a failed review?
 

eternal_noob

Aspiring Daemon

Reaction score: 403
Messages: 633

I can assume that the code that goes to HEAD is reviewed, and therefore the problem in this case is a failed review?
According to the article, "the landlord from hell" closed reviews without addressing the problems and completely ignoring them.

Slightly tangential note, but my questions to mmacy@ about original
wireguard commit (and ZoL, FWIW) or Phabricator comments had not been
answered; I also recall reviews being closed in "not accepted" state
by him.
While I appreciate the heavy-lifting, developers should be
ready to explain and sometimes defend their work on public forums.
 

troublemaker

Member

Reaction score: 5
Messages: 43

According to the article, "the landlord from hell" closed reviews without addressing the problems and completely ignoring them.


Uhmmmm.... okay, so if i get it correct:
  • every commit is reviewd by someone other than the committer
  • the committer decides when the code is done, and when it's fine to put it into HEAD
Does that mean that the reviewer(s) can still point out that there is dangerous code committed in the release?
Okay, maybe not ideal, but it would still be fine.
 

troublemaker

Member

Reaction score: 5
Messages: 43

The fact that a committer can close any review as "not accepted" is the main problem here. I mean, code reviews are there for a reason.
True that, but if the reviewers are aware that the review has been closed they can still do something.
Which is, I believe, what happened.
I mean, personally I would not allow code to be committed without reviewers approval. But the current way (again, if I get it correctly) would still do the job.
What surprised me is that Ars Technica speaks of unreviewed code, basically saying that it's kind of normal. But that doesn't look true. So basically I am looking at the generic situation now, not only at this specific case.

EDIT: Sorry, something else: 40000 lines of code is a hell of a review.
 

Snurg

Daemon

Reaction score: 589
Messages: 1,349

I'd like to quote danfe in the phabricator review:
danfe said:
On a larger scale, is adding another pile of crypto bits necessary? Would it be feasible to use existing crypto framework instead? I've heard Linux folks eventually forced the author to rewrite this zinc thing of his as a simple wrapper over kernel crypto API. What's the situation in our case?
Just scroll through the code and see yourself what an insane amount of redundant garbage code could have been thrown out in case the system crypto could have been used...
I didn't find any reaction to danfe's comment, and this puts me in a pensive mood.

IIRC, the most common cause is "commit bit taken in for safekeeping" - in other words, the committer has been absent from FreeBSD project work for a long time, or that the committer self asks for the commit bit to be taken in, because of ENOTIME to work on FreeBSD.
I am not aware of exact times when he had commit bit and when not.
He had it unrevoked apparently most or all the years he spent in prison.

After he left prison, he constantly changed his contact emails. That is highly unusual and warrants the question why.
Maybe his admission that he had not yet cleaned up the consequences of his crime more than a dozen years ago, when being interviewed by ArsTechnica, explains a lot. Especially when one looks at his slow identity change from "Kip Macy" to "Matthew Macy".

Further, I found very interesting info from April 2016, possibly connected with the FreeBSD 12 /boot/kernel and /boot/modules video driver disaster, where he seems to have been involved (no idea to which degree, though).

His last public activity as kmacy was on Nov 18, 2016 (note sbruno's next comment indicating that project seemed finished).
In the quarterly report 07/2017 is indicated that there was work going on with Macy.
The next mention at Jul 25 2017 on site:freebsd.org was then by the apparent client, already mentioning him as mmacy, but including the old account kmacy in the rewievers list.
Then, February 2018, his commit bit was reinstated, this time on his new account mmacy.

He was also, together with Sean Bruno, responsible for the botched Intel ethernet driver merge, which led to many driver functions becoming inaccessible (even though they are still being listed in the respective manual pages).

But the most delicious thing above all is probably his special kind of involvement, which also might partly explain why there are so few messages from him in public, which I personally find somewhat unusual for a "typical" developer.

His "working relationship" to the Core Team possibly might be characterized best by his own words in the phabricator:
Matt Macy said:
I've been asked to ...

What puts up the question what motivated him to join in 2005?
What made him such a valuable asset for the Core team, after power relations had settled after the FreeBSD God Wizard Matt Dillon was sent into the desert?
Was he a volunteer?
Or was he a hired helper with special permissions to bypass the normal due development course, either for vendors/sponsors or other things that nobody of the core team wanted to do themselves?

I wish there were more transparency, so there would be no need to lift up the rugs to look what's under them.
 

troublemaker

Member

Reaction score: 5
Messages: 43

Don't worry, FMLU much of that is
C:
#ifdef LINUX
... dead code ...
#endif /* LINUX */
In the FreeBSD code?
Interesting.

Well, I am aware that there are better ways, but I am not going to tell the FreeBSD guys how they have to manage their code.
I am good if that code reaches the HEAD, as long as it's taken out right after that.
 

obsigna

Daemon

Reaction score: 863
Messages: 1,262

. . ., as there were a lot of fans of those who wanted to see WireGuard in FreeBSD and PF Sense. There was a lot of encouragement to get it in by fans for some time, and putting it in at the last minute will always be prone to problems. . . .
This boils it down, what actually happened. Like a bus full of soccer fans and the bus driver prevented an accident in the last minute by kicking in the balls of the fanatics who had their grip already on the steering wheel.
 

Jose

Daemon

Reaction score: 903
Messages: 1,107

You're kidding right? Sure, you're right, nothing happened but it wasn't through review or testing, but rather luck that the author of wireguard looked at the code and made it public.
No luck involved. Another Freebsd committer reached out to Donenfeld (lead author of Wireguard) with concerns about the implementation.
The two of them plus an Openbsd developer that worked on the Openbsd WG implementation then worked together to come up with an alternate implementation.
 

Jose

Daemon

Reaction score: 903
Messages: 1,107

A criminal (served 4 years and 4 month)
A coward (fled to Italy to avoid prosecution)
A misanthropist (attempting to illegally evict tenants from a building he had bought)
A psychopath (forged extremely threatening emails appearing to be from the tenants themselves)

I'd revoke his commit bit.
So there's no possibility for rehabilitation, ever? Why don't we just throw everyone in prison forever for every crime?

Yes, what he did is despicable, especially the part where he ruined his parents. People don't often change, but I believe that hard time at San Quentin is the kind of wake-up call that could make someone change.

Also, Macy was heavily involved in moving the ZFS implementation to Openzfs. Are you going to stop using ZFS on Freebsd 13?
 

PMc

Daemon

Reaction score: 671
Messages: 1,354

Please compare the following two occurrences:
  • When I filed in a (truly minor) bug report recently (PR 253861), I was kindly asked to write a test to asure it could not happen again (because the genuine author is too busy commiting more flaws like that?). So, to proceed with an already existing minor bug, the reporter is requested to provide tests... versus:
  • FMLU that guy from the WireGuard project (Jason A. Donenfeld) wrote a bunch of tests for the FreeBSD in-kernel implementation, when several dozens kLoC have already been commited... So, to commit dozens of kLoC that conforms to numerous commonly accepted worst-practices of C programming, all you need is mentorship from Sean Bruno. Just let the others write the tests.
Now we get nearer to the issue.
There is a German saying: "die kleinen fängt man, die großen läßt man laufen". (look it up in a translator)

I very much believe that your PR was thoroughly written and well considered. Now the common sense of all this discussion seems to be that we need more thorough QA procedures. The discomfort with these is that they create additional (and often mostly formal) work before something is accomplished (and that work then gets pushed around, as in Your case). But if something is big enough and the right people are involved, it still can bypass the checks&balances.

One main difference between free software and commercial software used to be that free software is more agile (that was true long before "agile" became a buzzword): it once took me 23 hours to get a fix into the HEAD of sendmail, it took more than a year to get a fix into a big commercial unix.
The reason for that, as I think, was that in the free software those people were in charge who were concerned with the software - often those who had written it in the first place, while in business people in charge were concerned mainly with the business. (And this is a difference that you cannot overcome with "agile" methodology - which is why "agile" is mostly BS.)

But now a generation has passed, and the projects have become way too big for anybody to overlook it all, and also the old guys, who would be able to still overlook a good part of it, have retired.
That seems to me the actual issue; and as it seems, formal checks+balances, while not a really good solution, is the best that mankind has come up with for such kind of problems. And I honestly do not really know how to solve this.
 

kpedersen

Son of Beastie

Reaction score: 1,992
Messages: 2,863

A criminal (served 4 years and 4 month)
A coward (fled to Italy to avoid prosecution)
A misanthropist (attempting to illegally evict tenants from a building he had bought)
A psychopath (forged extremely threatening emails appearing to be from the tenants themselves)

I'd revoke his commit bit.

It may be a hard pill to swallow but even if a mass murderer wrote a good bit of software, it shouldn't really be rejected because of the person. Respect their skills, not their *ahem* life choices.

Sure, if the source code is a poor quality or makes a dialog appear telling you to be very nasty to your tenants, then that is a different matter...

But I guess I am a little bit cold when it comes to this stuff. I probably blame the fact that the UK is built entirely upon murder and torture that I have given up XD
 

Jose

Daemon

Reaction score: 903
Messages: 1,107

One main difference between free software and commercial software used to be that free software is more agile (that was true long before "agile" became a buzzword): it once took me 23 hours to get a fix into the HEAD of sendmail, it took more than a year to get a fix into a big commercial unix.
One of my most bitter professional memories is of when I discovered that I had introduced a regression into a release candidate. I further realized that this regression was only going to affect our largest and most important customers.

I approached management about making a simple change of a few lines to avert this problem. I was rebuffed because the process had to be followed. It was too late in the release cycle to make any changes. A few weeks later a super-hot customer escalation landed in my lap. Fortunately I had a code fix ready.

It seems at some point in most large companies the process becomes more important than the result. The means become more important than the ends after a certain kind of manager starts to get hired. Managers who care more about covering their ass than getting things done.

It's one of the reasons why I've tried to stick to smaller companies throughout my career. They have other pathologies, but I don't find them as obnoxious.
The reason for that, as I think, was that in the free software those people were in charge who were concerned with the software - often those who had written it in the first place, while in business people in charge were concerned mainly with the business. (And this is a difference that you cannot overcome with "agile" methodology - which is why "agile" is mostly BS.)
Don't get me started on the "agile" nonsense. There's so much ridiculousness there. I'm only going to quote a former colleague that liked to say "you don't run a marathon by breaking it up into a series of sprints".
 

vigole

Daemon

Reaction score: 1,418
Messages: 1,243

Update:
Ars' moved the article from feature section to the bottom of their blog page. Probably, they've received enough clicks from the clickbait. Ready for the next target.
Also, the writer -- I forgot his name, he's a blog writer for Ars -- RT some FreeBSD-related tweets on his twitter. Some sort of fair and balance signalling, I assume?!
 

mark_j

Daemon

Reaction score: 681
Messages: 1,192

Which has nothing to do with what I said. The system works. It works well. In this case, a hole was found--a bug in the system. The bug will be fixed but the
At the risk of a circular argument: It has everything to do with what you stated. There is no system unless your system is luck.

This is not some bug in a switch on ls(1), this is software designed to provide a secure connection from one server to another. It suffers from buffer overflows, for goodness sake, and by virtue of good luck it was found before it was placed into production. It should never have been anywhere near a release candidate. The software's not even alpha quality.



software was never released. In the meantime, nothing happened.

You are now stating FreeBSD's code, due to this one issue, is managed by luck which is a preposterous statement.

No, I am not saying that. Read what I said.
 

Jose

Daemon

Reaction score: 903
Messages: 1,107

At the risk of a circular argument: It has everything to do with what you stated. There is no system unless your system is luck.
At the risk of repeating myself, no luck was involved. Another Freebsd committer had misgivings about such a large commit with so little review and he sought and found help auditing it in the broader FOSS community.

Now the hard work of that committer and his two colleagues is being exploited by Ars Technica to sell ads on the Web. It's disgusting.

Edit: Exploitation with a nice side of scapegoating. Let's all enjoy a good, juicy two minutes hate on Macy.
 

Zirias

Daemon

Reaction score: 1,414
Messages: 2,468

At the risk of repeating myself, no luck was involved. Another Freebsd committer had misgivings about such a large commit with so little review and he sought and found help auditing it in the broader FOSS community.
Even if the initiative came that way (I can't verify) – you wouldn't sell that as the correct and safe procedure for such things? It was in -RC2, which is the final mandatory RC, so removing it thereafter was indeed the very latest possible moment. Luck is a factor, at least with this timing.
Let's all enjoy a good, juicy two minutes hate on Macy.
Whatever this article does… the takeaway for FreeBSD isn't fingerpointing towards Macy. He obviously "failed" here, but no single failure should lead to a near-desaster in a release. So, just forget that article, it's bad style in many ways, but don't forget the incident, so it won't repeat.
 
Top