sint64 corrupt read from wire

[denizzzka]

While (third-party created) wire binary reading sint64 values treated as unsigned. This causes what zagZig is not applied to it and values readed wrong.

For example, sint64 value 54 will be readed as 108

integration tests testing something by wrong way?

Field:

repeated sint64 id = 1 [packed = true];

@Proto(1, Wire.zigzag, Yes.packed) long[] id = protoDefaultValue!(long[]);

[dcarp]

This would be really strange. The zigzag encoding/decoding is tested in the conformance tests: https://github.com/protocolbuffers/protobuf/blob/e5f246d3a8b48fbf6123db4ba6b20639b20615dd/conformance/binary_json_conformance_suite.cc#L782

Can you provide a simplified unittest for it?

[denizzzka]

It is impossible to unittest, It is need to use some pbf blob sample (from google?) with all of supported stuff and try to decode it.

[denizzzka]

The zigzag encoding/decoding is tested in the conformance tests:

Maybe issue affects only negative values, not 12345? Or encoding and decoding again "leveling" each other?

[dcarp]

It is impossible to unittest, It is need to use some pbf blob sample (from google?) with all of supported stuff and try to decode it.

I think a unittest like this should demonstrate de problem.

[dcarp]

Maybe issue affects only negative values, not 12345? Or encoding and decoding again "leveling" each other?

If you look carefully, the decoding is tested in the context of encoding. But the encoding is tested against raw bytes. So there should be no "leveling". This means that if the encoding test are proven correct, then the decoding will also be proven correct without needing to work with raw bytes for that.

[denizzzka]

But what if first encoding is incorrect? (As for #22)

I’m having a Friday night, sorry, I’m probably not giving you the code now.

But for decoding I use the widely used OSM PBF data. And in this data I see a varints that does not undergo correct conversion. And I can convert it manually into right values by additional calling of appropriate zigZag function. And there are no complaints from other users of this data with other (non Dlang) libraries.

[dcarp]

If first the encoding is incorrect, then that test must be corrected. (BTW. issue 22 had no problem with encoding/deciding, but it was a code generation problem).

You don't need to show the code, but a short example that demonstrates the problem.

Mostly probable is the original data correct. I just proposed some steps to help us localize the problem.

[denizzzka]

Just create by third party software field value as described in https://github.com/dcarp/protobuf-d/issues/23#issue-456093737 and then try to read it by protobuf-d

[denizzzka]

Reproduce:

$ cat one_field.proto
syntax = "proto3";

message RootMsg {
   repeated sint64 varint_value = 1 [packed = true];
}

$ cat one_field.txt
varint_value: [54]

$ protoc --encode=RootMsg one_field.proto < one_field.txt > result.pbf

$ protoc --decode=RootMsg one_field.proto < result.pbf
varint_value: 54

Now result.pbf contains encoded signed 54 value.

Generate one_field.d:

$ protoc --plugin=../protobuf-d/build/protoc-gen-d --d_opt=message-as-struct --d_out=. one_field.proto

Next, prepare D source. I use single file project, so just copy-n-paste generated decoder one_field.d into it:

/+ dub.sdl:
name "test"
dependency "protobuf" version="~>0.5.3"
+/

//==========================
// Generated by the protocol buffer compiler.  DO NOT EDIT!
// source: one_field.proto

//~ module one_field; // commented out by me

import google.protobuf;

enum protocVersion = 3008000;

struct RootMsg
{
    @Proto(1, Wire.zigzag, Yes.packed) long[] varintValue = protoDefaultValue!(long[]);
}
//==========================

import std.file;
import std.stdio;

void main()
{
    auto blob = cast(ubyte[]) readText("result.pbf");
    auto msg = blob.fromProtobuf!RootMsg;
    msg.varintValue[0].writeln; // 108 instead of 54
}

Run:

dub run --single test.d
Performing "debug" build using /usr/bin/dmd for x86_64.
protobuf 0.5.3: target for configuration "protobuf" is up to date.
test ~master: building configuration "application"...
Linking...
To force a rebuild of up-to-date targets, run again with --force.
Running ./test 
108

Result is 108 :-(

[dcarp]

Thanks! I'll fix it tonight.