search

mapbox / pbf

A low-level, lightweight protocol buffers implementation in JavaScript.
BSD 3-Clause "New" or "Revised" License
796 stars 106 forks source link

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

Closed slam closed 5 years ago

slam commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

slam commented 5 years ago

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: https://github.com/mapbox/pbf/pull/96. Let me know if you would like to merge the PR or just pin to the old version of resolve-protobuf-schema.

mourner commented 5 years ago

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

slam commented 5 years ago

CI looks good...

mourner commented 5 years ago

Thanks so much!

slam commented 5 years ago

You are very welcome.

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

mourner commented 5 years ago

@slam published as v3.2.0.

slam commented 5 years ago

Thank you for the quick response. Really appreciate it.