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

Fix a corruption when raw message > 0x10000000 bytes #95

Merged
merged 1 commit into from
Mar 10, 2019

Conversation

slam
Copy link
Contributor

@slam slam commented Mar 8, 2019

This fixes a data corruption bug when writing a message that exceeds
0x10000000 bytes in size. The varint-encoded size in the beginning of
the message includes an extra byte.

Add a test for this condition.

@slam
Copy link
Contributor Author

slam commented Mar 8, 2019

The easiest way to fix the test/compile.test.js failures is to pin resolve-protobuf-schema to 2.0.0. Let me know if you would like me to do that.

diff --git a/package.json b/package.json
index c984b4c..9c012cd 100644
--- a/package.json
+++ b/package.json
@@ -37,7 +37,7 @@
   "homepage": "https://github.com/mapbox/pbf",
   "dependencies": {
     "ieee754": "^1.1.6",
-    "resolve-protobuf-schema": "^2.0.0"
+    "resolve-protobuf-schema": "2.0.0"
   },
   "devDependencies": {
     "benchmark": "^2.1.0",

@mourner
Copy link
Member

mourner commented Mar 9, 2019

Good catch — thank you! Can you say more about why resolve-protobuf-schema needs pinning — what was breaking in the new version?

@slam
Copy link
Contributor Author

slam commented Mar 9, 2019

resolve-protobuf-schema upgraded its dependency protocol-buffers-schema from ^2.0.2 to ^3.3.1. That new version tightens the validation of the packed fields to follow more closely the protobuf spec, causing some of the existing compile tests to fail with this message:

not ok Fields of type MessageType cannot be declared [packed=true]. Only repeated fields of primitive numeric types (types which use the varint, 32-bit, or 64-bit wire types) can be declared "packed". See https://developers.google.com/protocol-buffers/docs/encoding#optional

I'm not sure if that interpretation of the spec is strictly correct. Technically speaking, MessageType in the tests is an enum which uses the varint wire type.

I submitted a PR that fixes that broken tests: #96. Let me know if you would like to merge the PR or just pin to the old version of resolve-protobuf-schema.

@mourner
Copy link
Member

mourner commented Mar 10, 2019

Let's rebase on master and force-push to make sure the CI passes now.

This fixes a data corruption bug when writing a message that exceeds
0x10000000 bytes in size. The varint-encoded size in the beginning of
the message includes an extra byte.

Add a test for this condition.
@slam
Copy link
Contributor Author

slam commented Mar 10, 2019

CI looks good...

@mourner mourner merged commit b26dbbb into mapbox:master Mar 10, 2019
@mourner
Copy link
Member

mourner commented Mar 10, 2019

Thanks so much!

@slam
Copy link
Contributor Author

slam commented Mar 10, 2019

You are very welcome.

Could you please cut a release? Would like to be able to pull the fix in from npm.

@mourner
Copy link
Member

mourner commented Mar 11, 2019

@slam published as v3.2.0.

@slam
Copy link
Contributor Author

slam commented Mar 11, 2019

Thank you for the quick response. Really appreciate it.

@slam slam deleted the off-by-one-corruption branch March 11, 2019 16:27
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.

None yet

2 participants