Fix race in local storage #14888
Fix race in local storage #14888
Conversation
LocalStorage should only put completed files in position Signed-off-by: Andrew Thornton <art27@cantab.net>
|
There are a couple of other races in here - which I don't really know how to fix. The issues are:
|
GiteaBot
added
the
lgtm/need 2
|
This PR however does get rid of the most egregious problem. |
GiteaBot
added
lgtm/need 1
| @@ -46,9 +49,14 @@ func NewLocalStorage(ctx context.Context, cfg interface{}) (ObjectStorage, error | |||
| return nil, err | |||
| } | |||
|
|
|||
| if config.TemporaryPath == "" { | |||
There was a problem hiding this comment.
Put them to a system temp directory is better.
There was a problem hiding this comment.
These files may be large and the system temporary directory is not guaranteed to be able to cope.
The system tempdir may be on a different volume in Windows or may be on a different filesystem.
The only place we know for sure that LFS files will fit and should be renamed atomically is if they're in the LFS repository path.
The temporary directory is configurable - although I've not explicitly documented it here: set TEMPORARY_PATH for the storage
|
Fundamentally the other races exist because there is a verification step that the storage system is not aware of and isn't doing. But thinking on I suppose we could "buffer" the data being passed to the storage system so that by the time eof is called we know that the data we're passing has actually got the right SHA256 and is the right size. That is: instead of using a teereader we wrap the reader and write to the hash and keep account of the size during the Read(...) Then when EOF is returned from the body reader we check the hash and the size - determine if it's correct before returning io.EOF otherwise we return some other error. The other error will then percolate up to the storage reader and its io.copy killing them and aborting the storage. Boom. Verification in stream and no more races. |
|
Now it's likely that the body reader implements io.WriterTo which may mean we need to implement io.Writer in addition to WriterTo to do the same trick just slightly differently. |
GiteaBot
added
lgtm/done
LocalStorage should only put completed files in position Signed-off-by: Andrew Thornton <art27@cantab.net>
|
Backport -> #14901 |
|
Sorry was in bed so couldn't do the backport! |
Continuing on from #14888 The previous implementation has race whereby an incomplete upload or hash mismatch upload can end up in the ContentStore. This PR moves the validation into the reader so that if there is a hash error or size mismatch the reader will return with an error instead of an io.EOF causing the storage to abort the storage. Signed-off-by: Andrew Thornton <art27@cantab.net>
Backport go-gitea#14895 Continuing on from go-gitea#14888 The previous implementation has race whereby an incomplete upload or hash mismatch upload can end up in the ContentStore. This PR moves the validation into the reader so that if there is a hash error or size mismatch the reader will return with an error instead of an io.EOF causing the storage to abort the storage. Signed-off-by: Andrew Thornton <art27@cantab.net>
Backport #14895 Continuing on from #14888 The previous implementation has race whereby an incomplete upload or hash mismatch upload can end up in the ContentStore. This PR moves the validation into the reader so that if there is a hash error or size mismatch the reader will return with an error instead of an io.EOF causing the storage to abort the storage. Signed-off-by: Andrew Thornton <art27@cantab.net>
LocalStorage should only put completed files in position
Signed-off-by: Andrew Thornton art27@cantab.net