Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exit early if decompressed size exceeds OTSStream's size limit #244

Merged
merged 7 commits into from
Jul 8, 2022

Conversation

fred-wang
Copy link
Contributor

@fred-wang
Copy link
Contributor Author

cc @khaledhosny @jfkthame @drott

@fred-wang
Copy link
Contributor Author

@jfkthame gfxOTSExpandingMemoryStream will need a

      size_t SizeLimit() override { return m_Limit; }

if this is taken.

@khaledhosny
Copy link
Owner

I don’t like the fact that we now have two different limits, and one is configurable and the other is not.

@fred-wang
Copy link
Contributor Author

To be clear, I believe we already have two limits:

  1. OTS_MAX_DECOMPRESSED_FILE_SIZE which is configurable at build time as a #ifdef.
  2. OTSStream's (well MemoryStream/ExpandingMemoryStream) limit which is specified at runtime in the constructor (so more easily configurable for third party)

So this commit is only fixing the process functions to take into account (2), which is the limit configuration used by Firefox/Chromium.

What do you suggest?

@@ -58,6 +58,8 @@ class OTSStream {

virtual ~OTSStream() {}

virtual size_t SizeLimit() = 0;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d rename it size().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Existing methods in OTSStream seemed to use CamelCase but I see that the trival one chksum() does not.

Copy link
Owner

@khaledhosny khaledhosny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a second thought, it makes sense.

src/ots.cc Outdated Show resolved Hide resolved
src/ots.cc Outdated Show resolved Hide resolved
include/ots-memory-stream.h Outdated Show resolved Hide resolved
fred-wang and others added 4 commits July 8, 2022 03:32
Co-authored-by: خالد حسني (Khaled Hosny) <khaled@aliftype.com>
Co-authored-by: خالد حسني (Khaled Hosny) <khaled@aliftype.com>
Co-authored-by: خالد حسني (Khaled Hosny) <khaled@aliftype.com>
@khaledhosny khaledhosny merged commit 7325fec into khaledhosny:main Jul 8, 2022
@fred-wang
Copy link
Contributor Author

@khaledhosny thanks!

@fred-wang fred-wang deleted the ots-stream-size-limit branch July 8, 2022 09:41
@fred-wang
Copy link
Contributor Author

mmh, I'm getting this kind of warnings when trying to port to chromium, so I guess we should use override everywhere

../../third_party/ots/src/include/ots-memory-stream.h:23:16: error: 'WriteRaw' overrides a member function but is not marked 'override' [-Werror,-Winconsistent-missing-override]
virtual bool WriteRaw(const void *data, size_t length) {
^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants