Bug #4569
open
Default-constructed MetaInfo does not equal MetaInfo decoded with default values
Added by Junxiao Shi over 6 years ago. Updated about 5 years ago.
50%
Description
Snippet to reproduce:
// g++ -std=c++11 -o x x.cpp $(pkg-config --cflags --libs libndn-cxx)
#include <iostream>
#include <ndn-cxx/encoding/block.hpp>
#include <ndn-cxx/meta-info.hpp>
int
main()
{
static uint8_t WIRE[] = { 0x14, 0x03, 0x18, 0x01, 0x00 };
ndn::MetaInfo a(ndn::Block(WIRE, sizeof(WIRE)));
ndn::MetaInfo b;
std::cout << static_cast<int>(a == b) << std::endl;
return 0;
}
Expected: program prints 1, indicating a equals b.
Actual: program prints 0, indicating a does not equal b.
Updated by Junxiao Shi almost 6 years ago
With Packet03Transition permitting unrecognized non-critical TLV elements, this problem becomes acuter: what does operator== mean for an object that represents a TLV element?
It could be defined as either:
- Equality of semantics. Under this choice,
MetaInfo("1400"_block)equalsMetaInfo("1403 180100"_block)because they are semantically equivalent: the former omits ContentType and accepts its default value, while the latter includes ContentType with the default value. - Equality of TLV encoding, excluding unrecognized elements. Under this choice,
MetaInfo("1400"_block)does not equalMetaInfo("1403 180100"_block).MetaInfo("1403 180100"_block)equalsMetaInfo("1405 F000 180100"_block)whereF000is ignored. - Equality of TLV encoding, including unrecognized elements. Under this choice,
MetaInfo("1400"_block)andMetaInfo("1403 180100"_block)andMetaInfo("1405 F000 180100"_block)do not equal one another, despite they all have the same semantics.
This would be a design choice of ndn-cxx library and should be applied to all network-layer TLV element classes in the library. It is not a protocol design question.
Updated by Alex Afanasyev almost 6 years ago
In my opinion, it should be the third option (I believe it is now). Semantics is kind of irrelevant here, as you cannot interpret the semantics of application-defined elements.
As for the behavior. I think it is fine as is. It's basically a spec problem. The default value should not have had the explicit encoding, as it effectively has 2 different states representing the same thing.
Updated by Davide Pesavento almost 6 years ago
There's also option 4: remove operator== and operator!= for these classes.
I doubt there are many use cases that require them, beyond low-level wire encoding comparison (essentially a memcmp) that can easily use Block's operators.
Updated by Davide Pesavento over 5 years ago
- Status changed from New to In Progress
- Assignee set to Davide Pesavento
- % Done changed from 0 to 50
- Estimated time deleted (
1.00 h)
Updated by Davide Pesavento over 5 years ago
In https://gerrit.named-data.net/c/ndn-cxx/+/5505 (still WIP) I have:
- Removed
operator==andoperator!=from those classes where they are ambiguous or not well-defined:- Interest: use
matchesInterestandmatchesDatainstead - MetaInfo: definition is unsound/inconsistent, see issue description and note-1
- Data: because it includes MetaInfo and suffers from the same problems, also I don't see many valid use cases
- Link: definition was "inherited" from Data, hence it wasn't taking the delegation list into account and was giving wrong results
- Interest: use
- Kept the operators in the following cases, all of which have clear/intuitive semantics and/or have widespread usage in several codebases:
- Block
- Name and Component
- Delegation and DelegationList
- KeyLocator (this is debatable, and will probably need an update for v0.3 evolvability rules)
- AdditionalDescription and ValidityPeriod
- FaceUri
- All types implementing the NFD management protocol: they have consistent definitions and somewhat clear semantics, also if we're going to rewrite the protocol there's no point in changing them now
- Left Exclude and Selectors alone because they're obsolete.
- I'm not sure what to do with:
- PrefixAnnouncement
- SignatureInfo
- Signature (are we going to deprecate/remove this class?)
- Certificate: this "inherited" the operators from Data, being a subclass, although I'm not sure if that made sense. In any case,
operator==between certificates seems to be used in a few places, includingndnsec list; is there a sensible definition of equality between two certificates?
Updated by Junxiao Shi over 5 years ago
PrefixAnnouncement can have operator< and std::hash defined on the announced name, ignoring other fields. Its equality operator then derives from operator<. This allows putting PA into a collection.
Same applies to KiteRequest and KiteAck.
SignatureInfo and Signature do not need compare operators.
Every Certificate is usually signed.
Its operator< should compare full name of the Data packet; if full name is unavailable (such as a default-constructed object), use its name.
Its operator== is derived from operator<, and is equivalent to a wire encoding comparison if wire encoding exists.
The above answers are written without consulting existing implementations of compare operators, and thus are uninfluenced by such.
Updated by Davide Pesavento over 5 years ago
Junxiao Shi wrote:
PrefixAnnouncement can have
operator<andstd::hashdefined on the announced name, ignoring other fields.
operator< and std::hash are out of scope for this issue.
Its equality operator then derives from
operator<.
Ok, but note that this is different from the current definition.
SignatureInfo and Signature do not need compare operators.
I tend to agree.
Every Certificate is usually signed.
Itsoperator==is derived fromoperator<, and is equivalent to a wire encoding comparison if wire encoding exists.
If it's going to be equivalent to wire encoding equivalence then I'm not going to define an operator==. Users can call .wireEncode() and compare the returned Block, if that's what they want.
Updated by Junxiao Shi over 5 years ago
5505,3
https://travis-ci.org/yoursunny/ndn-cxx-breaks/builds/562316480
There are build failures in NFD, repo-ng, NLSR, ndncert, ndns.
NFD breaks on SigningInfo. Its equality operators should stay. It's not mentioned in note-5.
NLSR, ndncert, ndns break on Certificate. repo-ng breaks on Data. These usages should compare wireEncode or name instead.
Updated by Davide Pesavento over 5 years ago
Junxiao Shi wrote:
5505,3
https://travis-ci.org/yoursunny/ndn-cxx-breaks/builds/562316480
There are build failures in NFD, repo-ng, NLSR, ndncert, ndns.NFD breaks on SigningInfo. Its equality operators should stay. It's not mentioned in note-5.
SigningInfo's operator== uses SignatureInfo's operator==. The latter is gone so the former had to go as well. One alternative is to keep the operator for SignatureInfo and give it clear semantics, for instance we could compare the SignatureType and KeyLocator and ignore any other TLVs. Another alternative is to compare the .wireEncode() of the SignatureInfo embedded in SigningInfo.
NLSR, ndncert, ndns break on Certificate. repo-ng breaks on Data. These usages should compare wireEncode or name instead.
Yes. The question is whether comparing the name is correct and sufficient or we need to compare the full name (or .wireEncode() which is essentially equivalent).
Updated by Junxiao Shi over 5 years ago
NFD breaks on SigningInfo. Its equality operators should stay. It's not mentioned in note-5.
SigningInfo's
operator==uses SignatureInfo'soperator==. The latter is gone so the former had to go as well. One alternative is to keep the operator for SignatureInfo and give it clear semantics, for instance we could compare the SignatureType and KeyLocator and ignore any other TLVs. Another alternative is to compare the.wireEncode()of the SignatureInfo embedded in SigningInfo.
The problem comes from this constructor:
/**
* @param signatureInfo A semi-prepared SignatureInfo which contains other information except
* SignatureType and KeyLocator. If SignatureType and KeyLocator are
* specified, they may be overwritten by KeyChain.
*/
SigningInfo(SignerType signerType = SIGNER_TYPE_NULL,
const Name& signerName = getEmptyName(),
const SignatureInfo& signatureInfo = getEmptySignatureInfo());
This SignatureInfo was added in https://gerrit.named-data.net/c/ndn-cxx/+/2116/8/src/security/signing-info.hpp#111 to "handle more settings". A quick grep reveals that the current usage is limited to certificate issuance.
Ideally, SignatureInfo embedded in SigningInfo should be compared without SignatureType and KeyLocator elements, since these would be overwritten. However, comparing SignatureInfo::wireEncode() would be sufficient for current use case.
Updated by Davide Pesavento about 5 years ago
I've decided to split this task in (at least) two commits.
https://gerrit.named-data.net/c/ndn-cxx/+/5505 is the first part. It removes the operators for Interest, MetaInfo, and Signature.
Only a few NFD's unit tests are broken by this change, and those are easily fixed by https://gerrit.named-data.net/c/NFD/+/5666
https://gerrit.named-data.net/c/ndn-cxx/+/5665 is the second part and contains the more "complicated" removals. It's still a WIP so don't look at it yet.