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

There are many deficiencies #210

Closed
LuoYou1989 opened this issue May 27, 2021 · 6 comments
Closed

There are many deficiencies #210

LuoYou1989 opened this issue May 27, 2021 · 6 comments

Comments

@LuoYou1989
Copy link

There is no data range validation for the input data and no data range checking for the output data

@tomas-abrahamsson
Copy link
Owner

For input data validation, you might want to try the verify option.

Here's an example trying to encode -1 for an unsigned int:

$ cat m.proto
syntax='proto3';
message Msg { uint32 f = 1; }
$
$ erl
1> gpb_compile:file("m.proto", [maps,{verify,always}]).
...
3> m:encode_msg(#{f => -1}, 'Msg').
m:encode_msg(#{f => -1}, 'Msg').
** exception error: {gpb_type_error,{{value_out_of_range,uint32,unsigned,32},
                                     [{value,-1},{path,"Msg.f"}]}}
     in function  m:mk_type_error/3 (m.erl, line 353)
     in call from m:v_msg_Msg/3 (m.erl, line 332)
     in call from m:encode_msg/3 (m.erl, line 70)

For range checking for the output data, I'm not sure I follow. Could you explain more what you mean by this?

@LuoYou1989
Copy link
Author

$ cat ts.proto
import "t1.proto"
message video_c
{
required uint32 id = 1; // id
optional bytes name = 2; // name
repeated uint32 terms = 3; // terms
}

Administrator@PC-20170610DFEC MINGW64 /g/zzwz/proto/gpb (master)
$ erl -pa ebin
Eshell V8.3 (abort with ^G)
1> ts:decode_msg(<<>>, video_c).
{video_c,undefined,undefined,[]}
2>

@LuoYou1989
Copy link
Author

Another problem is that the generated code is too redundant, especially when a file is referenced multiple times, and there are N copies of the parsing code for that file.I don't know if there's a solution out there.

@tomas-abrahamsson
Copy link
Owner

I assume you did expect some kind of decoding error when the binary is missing required fields? Maybe that is a good addition, but it would need to be enabled by an option for backwards compatibility.

... too redundant when a file is referenced multiple times...

There is no option to generate one .erl per .proto. Some redesign would be needed for that. Some areas that may need thinking is extend and (proto3) defaults.

Is your case that you have a few top-level .proto, some 2nd-level .protos that are imported from the top-levels, and some 3rd or deeper level .protos? If so, do you generate .erls for all of these? If yes, then maybe you could reduce the overhead of duplication by generating only for the top-level .protos.

@LuoYou1989
Copy link
Author

thank you

@tomas-abrahamsson
Copy link
Owner

In 4.18.0, which i just pushed, I added an option, verify_decode_required_present (cmd-line: -vdrp) for generating code that checks that required fields are present in the input binary to decode, so I'm closing this issue. If there's anything more, feel free to re-open or open another issue.

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

No branches or pull requests

2 participants