search

ni / grpc-labview

gRPC client and server support for LabVIEW
MIT License
90 stars 62 forks source link

Repeated string or message fields with a field ID >= 2048 may not serialize correctly #393

Open jasonmreding opened 1 month ago

jasonmreding commented 1 month ago

I haven't actually tested if this works at all, but I believe it will still work but it might serialize incorrectly. The issue is with our implementation of ExpectTag which only looks at the first two bytes of the tag to see if the next element in the byte stream is for the same repeated field or not. So if the tag for the next field in the byte stream isn't the same but happens to have the same first two bytes, then we are going to interpret the data as another repeated element for the same field when it could be data for another field entirely. This could result in data corruption or possibly a crash.

AB#2906646

nischalks commented 1 month ago

@jasonmreding would you be taking a look at this issue ?

jasonmreding commented 1 month ago

@nischalks This isn't currently a priority for me even though I think the fix is probably relatively straight forward. Data serialization bugs are bad, but I think the real world chances of hitting this are pretty small. You would have to have field IDs over 2048 which I think is rare, and you would have to have two field IDs that are a multiple of 2048 apart with the same wire type that got serialized consecutively on the wire. I'm sure I can write a proto file to make that happen, but I imagine the real world chances of that occurring are pretty small. However, I'm happy to consult or review a PR if you are concerned about a more immediate fix. There are already grpc APIs called ExpectTag that handle this which can be copied from. They just need to be adapted from using a CodedInputStream to a char* as the input buffer.

nischalks commented 1 month ago

@jasonmreding at the moment we are not prioritizing to fix this issue.