-
Notifications
You must be signed in to change notification settings - Fork 16
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
Possible to use dproto in @safe code. #79
base: master
Are you sure you want to change the base?
Conversation
@@ -40,6 +42,8 @@ template TagId(alias T) | |||
template ProtoAccessors() | |||
{ | |||
|
|||
@safe: | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be applied to the templated methods; you can't guarantee it is appropriate.
I would suggest it only be applied to the non-templated serialize()
method. Attribute inference will take care of the rest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Didn't know about attribute inference :)
But still the templated deserialize()
has to be marked as @safe too. Is it appropriate to require that the ProtoInputRange R provides @safe functions? My answer is yes, you should definitively enforce bound checks when parsing data from non-trusted sources.
My earlier comment, that Apart from that, the commit message should be something meaningful explaining the rationale for the changes. |
if(isProtoOutputRange!R && | ||
(T.msgType == "double".msgType || T.msgType == "float".msgType)) | ||
{ | ||
r.put(src.nativeToLittleEndian!(BuffType!T)[]); | ||
} | ||
|
||
/// Ditto | ||
void writeProto(string T, R)(ref R r, const BuffType!T src) | ||
void writeProto(string T, R)(ref R r, const BuffType!T src) @trusted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the rationale for making these @trusted
. It in any case seems to me unwise in the case of a templated method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel that comfortable with @trusted
here either. Better would be @safe
which would require R.put()
to be either safe or trusted. So it would be the users choice.
This could break compatibility to older code.
Broadly, I'm concerned that this PR is just trying to get I speak here purely as a user, not a maintainer, but I would not be happy to see |
I don't see any sign of this in the diff ... ? |
@WebDrake: Thanks for having a look at this.
Partially true. For me it seems to be important that processing of non-trusted data cannot lead to memory corruptions.
Your eyes are right. Copy-paste accident on my side. |
Yes. This is why it's important to differentiate between code that actually is safe, versus code that merely compiles inside a
Note that the library's ability to provide genuine guarantees of safety may depend on the way the downstream user is using the library. It isn't necessarily the place of a library author to ban use-cases that are incompatible with
You should not be putting In this case, I think the problem with Note that if |
With this changes it should be possible to use structs generated with dproto in @safe functions. However, parsing protobuf code is still not @safe.
Removed deprecated inline imports.