5 ms·

Here is a writeup that provides a good review of the context for this [0]. 0. https://arstechnica.com/gadgets/2021/03/buffer-overruns-license-violations-and-ba

by tgbugs 6y ago

Here is a writeup that provides a good review of the context for this [0]. 0. https://arstechnica.com/gadgets/2021/03/buffer-overruns-license-violations-and-bad-code-freebsd-13s-close-call/ https://arstechnica.com/gadgets/2021/03/buffer-overruns-lice...

kevans91 6y ago
FWIW, "good review" is pretty debatable (source: was involved) -- it provides a decent overview, but you should read it with the thought in mind that the author did some pretty heavy cherry-picking to support their arguments. Also, this indeed is not "Kernel Debugging Quarterly."
- mbreese 6y ago
  I thought the Ars article was a pretty good overview for the context of this current issue and why there needs to be a statement on FreeBSD development practices at all. I don't expect Ars to get into too many kernel specifics, so won't knock them for that. But as far as bringing more attention to how the mess started in the first place (and was dealt with), I think the Ars article is a pretty good place to start. I mean, the question of how the rough-draft Wireguard implementation made it into the kernel in the first place without sufficient review is a pretty big question. I'm happy the FreeBSD team is addressing it directly.
  - kevans91 6y ago
    Yeah, sorry, I'm mostly lamenting that there's not a more objective overview. This whole situation is pretty tiring at this point, so it's unlikely that one will surface except as a post-mortem down the road.
    - busterarm 6y ago
      I think that unfortunately a lot of people can't separate "this code is bad" from "this person is bad". We all have times when we don't ship our best work. Life happens. It's also worth noting to the readers that Donenfeld's criticism of bad code is completely dispassionate and that he isn't above criticising himself. He seems to be genuinely among the nicest people in the community. This seems unfortunate for mmacy and unfortunately exacerbated by the behavior of Netgate.
      - loeg 6y ago
        > It's also worth noting to the readers that Donenfeld's criticism of bad code is completely dispassionate and that he isn't above criticising himself. Er, what? Donenfeld's hyperbole and wild characterizations are part of what fanned the flames and made this a tech-press mess instead of some quiet collaboration and bug reports.[0] > The first step was assessing the current state of the code the previous developer had dumped into the tree. It was not pretty. I imagined strange Internet voices jeering, “this is what gives C a bad name!” There were random sleeps added to “fix” race conditions, validation functions that just returned true, catastrophic cryptographic vulnerabilities, whole parts of the protocol unimplemented, kernel panics, security bypasses, overflows, random printf statements deep in crypto code, the most spectacular buffer overflows, and the whole litany of awful things that go wrong when people aren’t careful when they write C. While some details are based in reality, the paragraph goes well beyond the realm of truth. It makes totally unnecessarily jabs at Macy as "the previous developer." He is (broadly) a competent C/kernel developer. Yes, he did an inadequate job here. No, some Greek chorus isn't jeering about the C code just because stylistically it differs from how Donenfeld would write it. To my knowledge: * There was only a single "validation function that returned true," and it involved validating an ip address internal to a validated and decoded message from a wg peer. The message is already cryptographically verified; only peers that are part of the same mesh could spoof IPs outside of their configured range. (Donenfeld described this as validation functions, plural.) * Donenfeld's only ever found a single real buffer overflow. It's the one where Jumbo frames can cause heap overflow. His other buffer overflow claims are not realistic due to other constraints on the inputs. Mostly they seem to reflect stylistic preferences about using mallocarray(a, n) instead of malloc(a * n). So the claims of "spectacular" buffer overflow(s), plural, feels disingenuous. (I don't know what "spectacular" is supposed to mean in a cold technical critique, either.) Maybe this is "dispassionate," but it seems unnecessarily careless with the facts when writing technical criticism. To be clear, Netgate's press response to this was totally inappropriate and also just a dumb move. The public narrative would be more in their favor if they had been totally silent instead of posting the angry screed they did. Ars takes Donenfeld's hyperbole and runs with it, fact-checking only the easily verified claims. And there is some truthiness to it! Unfortunately, it's the rest of the communication that leaves something wanting. Anyway, I love wireguard and what Donenfeld has accomplished. I just wish the guy would be a bit more considerate and less colorful when writing sensitive emails. [0]: https://lists.zx2c4.com/pipermail/wireguard/2021-March/006494.html https://lists.zx2c4.com/pipermail/wireguard/2021-March/00649...
generalizations 6y ago
Honestly, the criticism of the freebsd code review process seems way overblown. To me, it looks like a smaller team that works well together in a less formal manner, and someone exploited that. They caught it, the person has lost trust, they all moved on. Big whup.
- unethical_ban 6y ago
  The items they discuss, like more rigor on MRs, static analysis, etc. seem like no-brainers from five years ago. I'm not knocking them, I don't know the scope of their limitations. And they're doing the improvements now, so good on them. Just seems odd some things weren't being done previously.
  - oblio 6y ago
    My guess, after lurking around the internet for the past few decades and keeping an eye on what BSDs generally do, it's a mix of NIH/people that don't want to change their existing workflows and lack of financial resources. It's hard to say what the percentages are for the two.
- gmueckl 6y ago
  Even if you trust your team members to do a good job, another pair of eyes on the code before it goes in rarely hurt. Everybody slips up once in a while and a review has at least a chance of catching the worst mistakes early. Team size doesn't matter. You just need to bring a little discipline to the process.
- ajross 6y ago
  Well, it's not the end of the world (and I feel much more sympathy for this guy's poor tenants than I do for pfSense customers), but... Bad code going unreviewed from a single author into a main branch from which people build production systems is definitely beyond "Big whup" severity. The equivalent would be if one person pushed an unreviewed driver into Fedora Rawhide or an Ubuntu Beta, say. It's a clear violation of the principles behind the service the distro is supposed to be providing for you. There are a zillion ways to make sure code gets reviewed before merge. Linux does it informally via Signed-off-by headers and a tree structure of trusted maintainers. Services like github provided automated tooling to enforce review. FreeBSD needs to just pick one. It's 2021, for goodness sake. A fixed COMMITTERS list just isn't going to cut it.
cryptonector 6y ago
Wow. Thanks for that link.
Ericson2314 6y ago
Wow, evil landlords, C being bad. All my favorite things to discuss in one article!
loeg 6y ago
I would suggest the LWN article instead: https://news.ycombinator.com/item?id=26572370 https://news.ycombinator.com/item?id=26572370

by tgbugs 6y ago

kevans91 6y ago

FWIW, "good review" is pretty debatable (source: was involved) -- it provides a decent overview, but you should read it with the thought in mind that the author did some pretty heavy cherry-picking to support their arguments. Also, this indeed is not "Kernel Debugging Quarterly."

mbreese 6y ago
I thought the Ars article was a pretty good overview for the context of this current issue and why there needs to be a statement on FreeBSD development practices at all. I don't expect Ars to get into too many kernel specifics, so won't knock them for that. But as far as bringing more attention to how the mess started in the first place (and was dealt with), I think the Ars article is a pretty good place to start. I mean, the question of how the rough-draft Wireguard implementation made it into the kernel in the first place without sufficient review is a pretty big question. I'm happy the FreeBSD team is addressing it directly.
- kevans91 6y ago
  Yeah, sorry, I'm mostly lamenting that there's not a more objective overview. This whole situation is pretty tiring at this point, so it's unlikely that one will surface except as a post-mortem down the road.
  - busterarm 6y ago
    I think that unfortunately a lot of people can't separate "this code is bad" from "this person is bad". We all have times when we don't ship our best work. Life happens. It's also worth noting to the readers that Donenfeld's criticism of bad code is completely dispassionate and that he isn't above criticising himself. He seems to be genuinely among the nicest people in the community. This seems unfortunate for mmacy and unfortunately exacerbated by the behavior of Netgate.
    - loeg 6y ago
      > It's also worth noting to the readers that Donenfeld's criticism of bad code is completely dispassionate and that he isn't above criticising himself. Er, what? Donenfeld's hyperbole and wild characterizations are part of what fanned the flames and made this a tech-press mess instead of some quiet collaboration and bug reports.[0] > The first step was assessing the current state of the code the previous developer had dumped into the tree. It was not pretty. I imagined strange Internet voices jeering, “this is what gives C a bad name!” There were random sleeps added to “fix” race conditions, validation functions that just returned true, catastrophic cryptographic vulnerabilities, whole parts of the protocol unimplemented, kernel panics, security bypasses, overflows, random printf statements deep in crypto code, the most spectacular buffer overflows, and the whole litany of awful things that go wrong when people aren’t careful when they write C. While some details are based in reality, the paragraph goes well beyond the realm of truth. It makes totally unnecessarily jabs at Macy as "the previous developer." He is (broadly) a competent C/kernel developer. Yes, he did an inadequate job here. No, some Greek chorus isn't jeering about the C code just because stylistically it differs from how Donenfeld would write it. To my knowledge: * There was only a single "validation function that returned true," and it involved validating an ip address internal to a validated and decoded message from a wg peer. The message is already cryptographically verified; only peers that are part of the same mesh could spoof IPs outside of their configured range. (Donenfeld described this as validation functions, plural.) * Donenfeld's only ever found a single real buffer overflow. It's the one where Jumbo frames can cause heap overflow. His other buffer overflow claims are not realistic due to other constraints on the inputs. Mostly they seem to reflect stylistic preferences about using mallocarray(a, n) instead of malloc(a * n). So the claims of "spectacular" buffer overflow(s), plural, feels disingenuous. (I don't know what "spectacular" is supposed to mean in a cold technical critique, either.) Maybe this is "dispassionate," but it seems unnecessarily careless with the facts when writing technical criticism. To be clear, Netgate's press response to this was totally inappropriate and also just a dumb move. The public narrative would be more in their favor if they had been totally silent instead of posting the angry screed they did. Ars takes Donenfeld's hyperbole and runs with it, fact-checking only the easily verified claims. And there is some truthiness to it! Unfortunately, it's the rest of the communication that leaves something wanting. Anyway, I love wireguard and what Donenfeld has accomplished. I just wish the guy would be a bit more considerate and less colorful when writing sensitive emails. [0]: https://lists.zx2c4.com/pipermail/wireguard/2021-March/006494.html https://lists.zx2c4.com/pipermail/wireguard/2021-March/00649...

generalizations 6y ago

Honestly, the criticism of the freebsd code review process seems way overblown. To me, it looks like a smaller team that works well together in a less formal manner, and someone exploited that. They caught it, the person has lost trust, they all moved on. Big whup.

unethical_ban 6y ago
The items they discuss, like more rigor on MRs, static analysis, etc. seem like no-brainers from five years ago. I'm not knocking them, I don't know the scope of their limitations. And they're doing the improvements now, so good on them. Just seems odd some things weren't being done previously.
- oblio 6y ago
  My guess, after lurking around the internet for the past few decades and keeping an eye on what BSDs generally do, it's a mix of NIH/people that don't want to change their existing workflows and lack of financial resources. It's hard to say what the percentages are for the two.
gmueckl 6y ago
Even if you trust your team members to do a good job, another pair of eyes on the code before it goes in rarely hurt. Everybody slips up once in a while and a review has at least a chance of catching the worst mistakes early. Team size doesn't matter. You just need to bring a little discipline to the process.
ajross 6y ago
Well, it's not the end of the world (and I feel much more sympathy for this guy's poor tenants than I do for pfSense customers), but... Bad code going unreviewed from a single author into a main branch from which people build production systems is definitely beyond "Big whup" severity. The equivalent would be if one person pushed an unreviewed driver into Fedora Rawhide or an Ubuntu Beta, say. It's a clear violation of the principles behind the service the distro is supposed to be providing for you. There are a zillion ways to make sure code gets reviewed before merge. Linux does it informally via Signed-off-by headers and a tree structure of trusted maintainers. Services like github provided automated tooling to enforce review. FreeBSD needs to just pick one. It's 2021, for goodness sake. A fixed COMMITTERS list just isn't going to cut it.

cryptonector 6y ago

Wow. Thanks for that link.

Ericson2314 6y ago

Wow, evil landlords, C being bad. All my favorite things to discuss in one article!

loeg 6y ago

I would suggest the LWN article instead: https://news.ycombinator.com/item?id=26572370 https://news.ycombinator.com/item?id=26572370