Fix masker data race #320
Fix masker data race #320
Conversation
There was a problem hiding this comment.
Good find! I completely looked over the fact that a read or a write to the buffer invalidates the slice.
Regarding the solution: this definitely works. But looking at the explanation you give in the PR, I would expect a different approach. upToIndex is only called in 3 places. In one of those, the output is ignored and in the other two, it is directly written to an io.Writer.
So could we avoid copying the whole buffer (which is potentially big) by just making sure that the Write also happens within the lock? Or wouldn't that solve the problem? Especially if the masker is processing a lot of output, it is a waste to having to copy every byte an extra time. Those are CPU cycles we're stealing from the main process that's running. Normally I'd say: don't care too much about this, but the masker is a special case where we have to find a balance between performance, readability of the code, and security.
This change ensures that the underlying buffer is protected by the mutex, by enforcing that all writes happen before the call to mutex.Unlock, while also ensuring high performance for the masker.
I was not aware of the need for having an efficient masker. I updated the |
internals/cli/masker/stream.go
Outdated
| _, err = s.dest.Write([]byte("<redacted by SecretHub>")) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| // Drop all bytes until the end of the mask. | ||
| _ = s.buf.upToIndex(i + int64(length)) | ||
| _, _ = s.buf.writeUpToIndex(ioutil.Discard, i+int64(length)) |
There was a problem hiding this comment.
In the current setup writeUpToIndex can never return an error here, but the function signature does not enforce this. Since flush() does have error as a return type, would it be a problem if we just forward the error? That way we prevent any bugs when writeUpToIndex is changed so that it can return when a ioutil.Discarder is provided as an argument.
There was a problem hiding this comment.
Yes, good point. This seems like a place where we could easily introduce a bug by mistake later on unless we handle the error.
There was a problem hiding this comment.
I added the error handling.
The
indexedBufferwas not goroutine safe, as theupToIndexmethod returned the result ofbytes.Buffer.Next, which is a slice of the underlying buffer. This slice, according to the documentation, is only valid until the next read or write operation on the buffer.Therefore it might happen that after calling
upToIndexbut before using the value it returns, another goroutine writes to the buffer and invalidates the result ofupToIndex.This issue can also cause a data race, as the access to the underlying buffer through the result of
upToIndexis not guarded with a mutex.This PR fixes the issue by updating the
upToIndexmethod to directly write these bytes to a writer, thus ensuring that any access to the underlying buffer is protected with the mutex.