Hacker News Reader

Comment by terom | Hacker News Reader

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

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/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.

View on HN

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

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-wit...

Brief discussion on revocation exemption requests: 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.

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.

I'm not sure I understand, it's a business logic bug, would have happen in any language.

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.

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.

> 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".

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.

> 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?

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.

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.

Most importantly, programmers of the 2020's shouldn't be making these decisions.

Wether you choose to pass by reference or copy, that should just indicate your desired semantics. The underlying compiler can decide if it's really worth making a copy or not.

Sadly most compilers won't elide out most unnecessary copy operations, even when performance would be improved to do so.

> Won't the pointer also be hot in cache in this case?

Depends on the situation; the parent was somewhat vague.

As one example, suppose you've previously created a `struct foo`, but you haven't accessed it in a long time; then fetching any of its fields will probably be a cache miss. But if it contains a pointer, fetching data from that pointer will probably be a second cache miss – which can't even be started until the first one completes, because the CPU doesn't know what the pointer is. If `struct foo` contains the data directly, there's a good chance it all fits in a single cache line (typically 64 bytes). But even if it's split across, say, two cache lines, the CPU knows which cache lines need to be accessed, so it can send off the second request without waiting for the first one to complete.

That said:

- Strings are variable length, so they usually can't be stored in place anyway, although some languages/frameworks have "small string optimization", meaning that sufficiently short strings are stored in the space that would otherwise be used for the pointer and length.

- In general, Swift is hardly a good role model when it comes to performance. :p

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).

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/55957a99e800baa...

I'll cut the LE team some slack on this one :) the PR does have tests

Sure. But it's now Tuesday. They should have gone back and edited in links to all the relevant documentation.

Reply on news.ycombinator.com