Enable peekable-based binary sniffing on ZipArchiveArtifacts #2773
Conversation
scottoneil-ms
requested review from
michaelcfanning,
suvamM,
shaopeng-gh,
rwoll,
yongyan-gh,
LingZhou-gh,
EasyRhinoMSFT and
cfaucon
as code owners
| string extension = Path.GetExtension(Uri.ToString()); | ||
| if (this.binaryExtensions.Contains(extension)) | ||
| const int PeekWindowBytes = 1024; | ||
| var peekable = new PeekableStream(this.Stream, PeekWindowBytes); |
There was a problem hiding this comment.
No, peekable just has the underlying stream and a managed buffer. 1k, in this case.
| string extension = Path.GetExtension(Uri.ToString()); | ||
| return this.binaryExtensions.Contains(extension); | ||
| GetArtifactData(); | ||
| return this.bytes != null; |
There was a problem hiding this comment.
Before this check was cheap, now it looks like it's potentially much more expensive since you're introspecting the actual stream. Are there any concerns around perf changes here?
There was a problem hiding this comment.
Sort of, but that expectation has to be baked into the whole premise of categorizing based on contents.
I would say I just gained appreciation on another thread that the way we protect ourselves from scanning obviously bogus content is the file deny extensions, which remains a feature. So you could imagine that once you've passed that check, we're going to scan, so the only perf-sensitivity to this change would be when we do the first read. (Not if.) That's not a high-stakes question.
Interested in Michael's take on this.
No description provided.