Bug #3200
closed
Type mismatch in Selectors, MetaInfo, SignatureInfo, and ControlResponse
Added by Davide Pesavento about 9 years ago. Updated about 7 years ago.
100%
Description
The NDN Packet Format spec defines {Min,Max}SuffixComponents and ChildSelector as nonNegativeInteger, which can be up to 8 octets long.
However, in ndn-cxx's Selectors class, these fields are declared as int, which is usually 32 bits long.
Same problem occurs in MetaInfo::m_type, SignatureInfo::m_type, and ControlResponse::m_code.
Updated by Junxiao Shi about 9 years ago
- Description updated (diff)
- Priority changed from Normal to Low
In theory, this is a problem for {Min,Max}SuffixComponents; but in practice, this problem is insignificant because ndn-cxx limits packet size to 8800 octets, so that number of NameComponents is restricted.
ChildSelector is no problem at all, because the only valid values are LEFTMOST=0 and RIGHTMOST=1.
I'd prefer declaring an enum of {NONE,LEFTMOST,RIGHTMOST} instead of using int.
Updated by Davide Pesavento about 9 years ago
Sure, this is mostly a theoretical problem, and won't happen in practice with valid packets. However there's also a minor implementation bug in the decoding function. Currently we have:
m_minSuffixComponents = readNonNegativeInteger(*val);
This code has an (implicit) narrowing conversion from uint64_t to int (not sure why the compiler doesn't warn about this). I believe the behavior in this case is implementation-defined, in particular I think the 32 least-significant bits will be assigned to m_minSuffixComponents and the rest will be discarded. In any case this is ugly and non-portable, and results in accepting packets that we should probably treat as invalid (values are too big). Even if we accepted them, we should not randomly truncate the values.
Updated by Davide Pesavento about 9 years ago
Same problem in ControlResponse:
uint32_t m_code;
...
m_code = readNonNegativeInteger(*val);
Updated by Junxiao Shi over 7 years ago
- Related to Bug #3974: Unsafe casts in ndn::nfd::*::wireDecode() added
Updated by Davide Pesavento over 7 years ago
- Related to deleted (Bug #3974: Unsafe casts in ndn::nfd::*::wireDecode())
Updated by Davide Pesavento over 7 years ago
Updated by Junxiao Shi over 7 years ago
I don't see the difference. readNonNegativeIntegerAs introduced in https://gerrit.named-data.net/4083 does range checking so there's no more narrowing conversion.
Updated by Junxiao Shi over 7 years ago
- Status changed from New to In Progress
- Assignee set to Junxiao Shi
- Target version set to v0.6
- % Done changed from 0 to 50
- Estimated time set to 1.50 h
https://gerrit.named-data.net/4105 Selectors, MetaInfo, and ControlResponse.
Same problem also appears in SignatureInfo::m_type but it's complicated because out-of-range numbers are used in test cases.
A default-constructed SignatureInfo initializes m_type to -1, which encodes to 2^64-1.
If I change the decoding procedure to use readNonNegativeIntegerAs<int32_t>, the field cannot decode successfully.
Ideally, SignatureInfo::wireEncode should fail if m_type is invalid.
However, that change breaks many test cases, and breaks operator==(Data, Data) for default-constructed Data.
I'll fix those in another commit.
Updated by Junxiao Shi over 7 years ago
Regarding Selectors, it was suggested that uint64_t instead of int should be used to represent MinSuffixComponents and MaxSuffixComponents fields.
By restricting the range here you're basically declaring the packet invalid, even though it's not invalid according to the NDN-TLV spec. This is especially true for MaxSuffixComponents, where a very large value, although suboptimal, wouldn't break anything.
The practical problem is: so far, the special value -1 indicates MinSuffixComponents or MaxSuffixComponents is unspecified. If we switch to uint64_t, -1 becomes infeasible.
Instead, I'll have to declare the type as optional<uint64_t>, and update NFD code accordingly.
Given no practical usage for large MinSuffixComponents and MaxSuffixComponents values, I think it's better to leave it as int for now, and do the type change later.
Updated by Davide Pesavento over 7 years ago
Junxiao Shi wrote:
I don't see the difference.
readNonNegativeIntegerAsintroduced in https://gerrit.named-data.net/4083 does range checking so there's no more narrowing conversion.
They're two different problems. The behavior in the two cases is dictated by separate rules of the C++ language.
However, given that readNonNegativeIntegerAs can solve both, I will re-add the "related-to".
Updated by Davide Pesavento over 7 years ago
- Related to Bug #3974: Unsafe casts in ndn::nfd::*::wireDecode() added
Updated by Junxiao Shi over 7 years ago
- Subject changed from Type mismatch in {Min,Max}SuffixComponents and ChildSelector to Type mismatch in Selectors, MetaInfo, SignatureInfo, and ControlResponse
- Description updated (diff)
While working on #4171-10, I noticed one more problem related to SignatureInfo::m_type: Signature::getType is casting this int32_t into uint32_t.
My plan is to change the return type of Signature::getType to tlv::SignatureTypeValue, and throw an exception if SignatureInfo is invalid, given Signature::operator bool is already provided to tell whether SignatureInfo is valid.
This will result in a slight change in how Data is printed. It will change from "Signature: (type: 1, value_length: 128)" to "Signature: (type: SignatureSha256WithRsa, value_length: 128)".
I'll continue working on this after https://gerrit.named-data.net/4118 is merged.
Updated by Junxiao Shi over 7 years ago
https://gerrit.named-data.net/4134 fixes the problem for SignatureInfo::m_type.
Updated by Junxiao Shi about 7 years ago
- Status changed from In Progress to Closed
- % Done changed from 50 to 100