4 ms·

For context in terms of what caused this, here's the PR which I assume fixed the bug in question: https://github.com/letsencrypt/boulder/pull/4690 https://githu

by terom 7y ago

For context in terms of what caused this, here's the PR which I assume fixed the bug in question: https://github.com/letsencrypt/boulder/pull/4690 https://github.com/letsencrypt/boulder/pull/4690 It looks like a nasty and subtle pass-by-reference of a for-range local variable, although I'm having trouble figuring out where the reference is stored: https://github.com/letsencrypt/boulder/blob/542cb6d2e06e756aeeca964965ae6fc04c3dc62a/sa/sa.go#L1409 https://github.com/letsencrypt/boulder/blob/542cb6d2e06e756a... I've spent plenty of time hunting down similar bizarre bugs in Go code as well, where the called function ~implicitly~ takes a pointer to the iteration variable and stores it somewhere. Each iteration of the for loop updates the stack-local in-place, and later reads of the stored reference will not read the original value. It's hard to spot from the actual call site :/ EDIT: This was an explicitly taken `&v` reference, but the same thing can also happen implicitly, if you call a `func (x *T) ...` method on the variable.

heavenlyblue 7y ago
People say Rust’s borrowing rules are only useful for multi-threaded environments. This is one of those issues that they are supposed to solve.
- fortran77 7y ago
  "People" say that? Really? Now you build up straw men so you can slip "Rust" into every discussion.
  - steveklabnik 7y ago
    An example from less than a day ago: https://news.ycombinator.com/item?id=22466354 https://news.ycombinator.com/item?id=22466354
    - tedunangst 7y ago
      I'm not super confident I understand the bug, but it looks like sequential access to the reference. If I'm not mistaken, a mutable borrow in rust would end up with the same bug.
      - steveklabnik 7y ago
        I have not dug into the details enough to say if this is a bug Rust would prevent or not, I am only responding to the claim that people do not sometimes suggest that Rust's complexity only matters in the multi-threaded case.
        tedunangst 7y ago
        Gotcha.
      - m-n 7y ago
        If I understand correctly, each `authzPB` collected in the iteration stores references to fields of an `authzModel`. Before the patch, these were identical, referring to the fields of the loop variable v. Each iteration of the loop, v is set, and all those stored references pointed to the new value. Rust does give a compilation error for that.
        tedunangst 7y ago
        That makes sense.
      - fortran77 7y ago
        I think you're right, too.
  - webappguy 7y ago
    Do you take issue with Rust?
  - deleted 7y ago
    [deleted]
- deleted 7y ago
  [deleted]
- Thaxll 7y ago
  I'm not sure I understand, it's a business logic bug, would have happen in any language.
  - SolarNet 7y ago
    Except that the reference in question would have caused a lifetime error in rust which would have required the developrs to explictly acknowledge the choice they were making, likely by changing a bunch of types. Yes you could still do it in rust, but any reviwer of the code would say "why in the world are you doing it this way" because it would be forced into a complex cross call monstrosity.
  - chc 7y ago
    How do you figure this is a business logic bug? It looks like a pretty clear-cut implementation bug to me. Rust would 100% have caught this bug, and in fact I'm pretty sure it would have caught the bug at least two different ways: 1. The reference outlives the original value. 2. You can't have multiple mutable references at the same time.
    - cesarb 7y ago
      > 2. You can't have multiple mutable references at the same time. If I understood the issue correctly, only one of the references would be a mutable one, so the way Rust could have caught the bug would instead be the related rule: "you can't have an immutable reference and a mutable reference at the same time".
rcaught 7y ago
Is it just me or is the PR and the associated linking really lacking? The PR doesn't have a description and neither it or the commits link back to the original communication (or vise versa).
- terom 7y ago
  For even more context, this seems to have been on a Friday night (assuming US West coast) with production down: https://letsencrypt.status.io/pages/incident/55957a99e800baa4470002da/5e59d5d7a9ba9d04c65eb77f https://letsencrypt.status.io/pages/incident/55957a99e800baa... I'll cut the LE team some slack on this one :) the PR does have tests
  - londons_explore 7y ago
    Sure. But it's now Tuesday. They should have gone back and edited in links to all the relevant documentation.
gwd 7y ago
What's particularly unfortunate about this is the comment just above the call: // Make a copy of k because it will be reassigned with each loop. But v is reassigned with each loop too. The real question is why there's so much pass-by-reference in the first place. K looks to be a domain name string -- it's almost certainly faster to copy it than to dereference it everywhere.
- thenewnewguy 7y ago
  > The real question is why there's so much pass-by-reference in the first place. K looks to be a domain name string -- it's almost certainly faster to copy it than to dereference it everywhere. I don't program in rust, so my knowledge here is limited to what these words mean in C/C++ - however shouldn't making a copy still require dereferencing the copy?
  - gwd 7y ago
    This is actually in Go, but the issue is the same. Suppose you have a struct like this: struct foo { struct bar elem } s; If you know the address of `s`, you just calculate the address of 'elem' from it and read the contents; a single memory read, all the data together cache-wise. Suppose on the other hand you have a struct like this: struct foo { struct bar *elemptr; } s; If you know the address of `s`, you have to first read `elemptr`, and only then read the value of `elem`. That's an extra memory fetch, and probably from a different part of the memory than `elem` is from. Copying on modern processors is very fast, and the resulting copy will be "hot" in your cache. So conventional wisdom I've heard is that unless `struct bar` is quite large (I've heard people say hundreds of bytes), it's probably faster to just copy the whole structure around than to copy the pointer to it around and dereference it. Caveat: I haven't run the numbers myself, but I've heard it from several independent sources; including, for instance, Apple's book on Swift.
    - jandrese 7y ago
      Won't the pointer also be hot in cache in this case? I only ask because it seems to me like excessive data copying (and cache eviction) is a major source of slowness in modern programs. People are churning their cache to pieces by copying the world for every function call. It's fine as long as your entire program fits neatly in cache, but once you exceed the cache size performance goes to hell because you force loads of misses of slightly-older data by constantly copying your working data.
terom 7y ago
LE just posted their own (excellent!) incident report of this on the mozilla bugtracker, including the discovery timeline, analysis of the bug, and follow-up steps: https://bugzilla.mozilla.org/show_bug.cgi?id=1619047#c1 https://bugzilla.mozilla.org/show_bug.cgi?id=1619047#c1 The original bug report, which was initially diagnosed as only affecting the error messages, not the actual CAA re-checking: https://community.letsencrypt.org/t/rechecking-caa-fails-with-99-identical-subproblems/113517 https://community.letsencrypt.org/t/rechecking-caa-fails-wit... Brief discussion on revocation exemption requests: https://bugzilla.mozilla.org/show_bug.cgi?id=1619179 https://bugzilla.mozilla.org/show_bug.cgi?id=1619179 Tomorrow will tell if granting a revocation exemption might have been a good idea in hindsight.

by terom 7y ago

heavenlyblue 7y ago

People say Rust’s borrowing rules are only useful for multi-threaded environments. This is one of those issues that they are supposed to solve.

fortran77 7y ago
"People" say that? Really? Now you build up straw men so you can slip "Rust" into every discussion.
- steveklabnik 7y ago
  An example from less than a day ago: https://news.ycombinator.com/item?id=22466354 https://news.ycombinator.com/item?id=22466354
  - tedunangst 7y ago
    I'm not super confident I understand the bug, but it looks like sequential access to the reference. If I'm not mistaken, a mutable borrow in rust would end up with the same bug.
    - steveklabnik 7y ago
      I have not dug into the details enough to say if this is a bug Rust would prevent or not, I am only responding to the claim that people do not sometimes suggest that Rust's complexity only matters in the multi-threaded case.
      - tedunangst 7y ago
        Gotcha.
    - m-n 7y ago
      If I understand correctly, each `authzPB` collected in the iteration stores references to fields of an `authzModel`. Before the patch, these were identical, referring to the fields of the loop variable v. Each iteration of the loop, v is set, and all those stored references pointed to the new value. Rust does give a compilation error for that.
      - tedunangst 7y ago
        That makes sense.
    - fortran77 7y ago
      I think you're right, too.
- webappguy 7y ago
  Do you take issue with Rust?
- deleted 7y ago
  [deleted]
deleted 7y ago
[deleted]
Thaxll 7y ago
I'm not sure I understand, it's a business logic bug, would have happen in any language.
- SolarNet 7y ago
  Except that the reference in question would have caused a lifetime error in rust which would have required the developrs to explictly acknowledge the choice they were making, likely by changing a bunch of types. Yes you could still do it in rust, but any reviwer of the code would say "why in the world are you doing it this way" because it would be forced into a complex cross call monstrosity.
- chc 7y ago
  How do you figure this is a business logic bug? It looks like a pretty clear-cut implementation bug to me. Rust would 100% have caught this bug, and in fact I'm pretty sure it would have caught the bug at least two different ways: 1. The reference outlives the original value. 2. You can't have multiple mutable references at the same time.
  - cesarb 7y ago
    > 2. You can't have multiple mutable references at the same time. If I understood the issue correctly, only one of the references would be a mutable one, so the way Rust could have caught the bug would instead be the related rule: "you can't have an immutable reference and a mutable reference at the same time".

rcaught 7y ago

Is it just me or is the PR and the associated linking really lacking? The PR doesn't have a description and neither it or the commits link back to the original communication (or vise versa).

terom 7y ago
For even more context, this seems to have been on a Friday night (assuming US West coast) with production down: https://letsencrypt.status.io/pages/incident/55957a99e800baa4470002da/5e59d5d7a9ba9d04c65eb77f https://letsencrypt.status.io/pages/incident/55957a99e800baa... I'll cut the LE team some slack on this one :) the PR does have tests
- londons_explore 7y ago
  Sure. But it's now Tuesday. They should have gone back and edited in links to all the relevant documentation.

gwd 7y ago

What's particularly unfortunate about this is the comment just above the call: // Make a copy of k because it will be reassigned with each loop. But v is reassigned with each loop too. The real question is why there's so much pass-by-reference in the first place. K looks to be a domain name string -- it's almost certainly faster to copy it than to dereference it everywhere.

thenewnewguy 7y ago
> The real question is why there's so much pass-by-reference in the first place. K looks to be a domain name string -- it's almost certainly faster to copy it than to dereference it everywhere. I don't program in rust, so my knowledge here is limited to what these words mean in C/C++ - however shouldn't making a copy still require dereferencing the copy?
- gwd 7y ago
  This is actually in Go, but the issue is the same. Suppose you have a struct like this: struct foo { struct bar elem } s; If you know the address of `s`, you just calculate the address of 'elem' from it and read the contents; a single memory read, all the data together cache-wise. Suppose on the other hand you have a struct like this: struct foo { struct bar *elemptr; } s; If you know the address of `s`, you have to first read `elemptr`, and only then read the value of `elem`. That's an extra memory fetch, and probably from a different part of the memory than `elem` is from. Copying on modern processors is very fast, and the resulting copy will be "hot" in your cache. So conventional wisdom I've heard is that unless `struct bar` is quite large (I've heard people say hundreds of bytes), it's probably faster to just copy the whole structure around than to copy the pointer to it around and dereference it. Caveat: I haven't run the numbers myself, but I've heard it from several independent sources; including, for instance, Apple's book on Swift.
  - jandrese 7y ago
    Won't the pointer also be hot in cache in this case? I only ask because it seems to me like excessive data copying (and cache eviction) is a major source of slowness in modern programs. People are churning their cache to pieces by copying the world for every function call. It's fine as long as your entire program fits neatly in cache, but once you exceed the cache size performance goes to hell because you force loads of misses of slightly-older data by constantly copying your working data.

terom 7y ago

LE just posted their own (excellent!) incident report of this on the mozilla bugtracker, including the discovery timeline, analysis of the bug, and follow-up steps: https://bugzilla.mozilla.org/show_bug.cgi?id=1619047#c1 https://bugzilla.mozilla.org/show_bug.cgi?id=1619047#c1 The original bug report, which was initially diagnosed as only affecting the error messages, not the actual CAA re-checking: https://community.letsencrypt.org/t/rechecking-caa-fails-with-99-identical-subproblems/113517 https://community.letsencrypt.org/t/rechecking-caa-fails-wit... Brief discussion on revocation exemption requests: https://bugzilla.mozilla.org/show_bug.cgi?id=1619179 https://bugzilla.mozilla.org/show_bug.cgi?id=1619179 Tomorrow will tell if granting a revocation exemption might have been a good idea in hindsight.