Opened 8 years ago

Closed 8 years ago

#1754 closed defect (wontfix)

small issues in LabelSequence

Reported by: jinmei Owned by: UnAssigned
Priority: medium Milestone: Year 3 Task Backlog
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 4 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

  • case consideration of equals() seems to be reversed.
        /// Performs a (optionally case-insensitive) comparison between this
        /// LabelSequence and another LabelSequence.
    ...
        bool equals(const LabelSequence& other, bool case_sensitive = false) const;
    
    Unless 'case_sensitive = true' means case insensitive (if so it's confusing, and would not be a good default anyway, because name comparison is case insensitive in most cases).
  • The actual implementation of equal() shold also be adjusted:
        if (case_sensitive) {
            return (strncasecmp(data, other_data, len) == 0);
        } else {
            return (strncmp(data, other_data, len) == 0);
        }
    
  • Also, strncasecmp() is not in the C++ standard library and it's less portable. And, while it happens to be okay today, I'm afraid it's a bit fragile to assume the label length field can be compared as a case insensitive value.
  • Maybe pedantic, but I'm afraid using char* is not technically correct because size of char may not necessarily be 8-bit (and with this class we'd use the pair of char* + its "size" as a 8-bit wire-format sequence.

(As a hindsight using std::string (which is basic_string<char>) as the
internal representation of Name was also a bad choice, and I've been
wanting to fix it but missing the chance. Maybe we should clarify
that, e.g., by changing to basic_string<uint8_t> at this opportunity).

Subtickets

Change History (6)

comment:1 in reply to: ↑ description ; follow-up: Changed 8 years ago by vorner

Hello

Replying to jinmei:

  • Also, strncasecmp() is not in the C++ standard library and it's less portable. And, while it happens to be okay today, I'm afraid it's a bit fragile to assume the label length field can be compared as a case insensitive value.

Hmm, interesting point. I think we could create two names that would not be equal because of different lengths of the labels (but the whole length being equal), however this would compare them as equal.

  • Maybe pedantic, but I'm afraid using char* is not technically correct because size of char may not necessarily be 8-bit (and with this class we'd use the pair of char* + its "size" as a 8-bit wire-format sequence.

(As a hindsight using std::string (which is basic_string<char>) as the
internal representation of Name was also a bad choice, and I've been
wanting to fix it but missing the chance. Maybe we should clarify
that, e.g., by changing to basic_string<uint8_t> at this opportunity).

I'd like to point out that the standard guarantees that sizeof(char) == 1. That is, it doesn't have to be 8 bit, because one byte could be 7 or 9 or something. But in that case, you won't have uint8_t anyway (the existence of fixed-width types is optional), so we wouldn't work anyway. The only thing I'd worry is that char might be signed, in which case using it as the length of sequence might be problematic.

comment:2 in reply to: ↑ 1 Changed 8 years ago by jinmei

Replying to vorner:

  • Also, strncasecmp() is not in the C++ standard library and it's less portable. And, while it happens to be okay today, I'm afraid it's a bit fragile to assume the label length field can be compared as a case insensitive value.

Hmm, interesting point. I think we could create two names that would not be equal because of different lengths of the labels (but the whole length being equal), however this would compare them as equal.

As long as the "label length" field is always <= 63, this cannot
happen (except for a compression pointer, which should be excluded in
this context), because it's smaller than 'A'. But if and when
the IETF introduces another extension like now-deprecated bit-string
labels, we'll encounter a trouble. Of course, we'll then have to
modify a lot of other parts of the code, but the use of strncasecmp()
would make the dependency implicit and difficult to identify.4

We already have a home-brew version of comparison in the Name class:
Name::equals(). I think it's better to share the main logic of this
code for both Name and LabelSequence? somehow. strncasecmp() would
need to do compare each character one by one anyway, so performance
wise it won't be much different (and actually Name::equals() could be
faster because it has shortcuts like comparing the number labels).

  • Maybe pedantic, but I'm afraid using char* is not technically correct because size of char may not necessarily be 8-bit (and with this class we'd use the pair of char* + its "size" as a 8-bit wire-format sequence.

(As a hindsight using std::string (which is basic_string<char>) as the
internal representation of Name was also a bad choice, and I've been
wanting to fix it but missing the chance. Maybe we should clarify
that, e.g., by changing to basic_string<uint8_t> at this opportunity).

I'd like to point out that the standard guarantees that sizeof(char)

1.

Right, but it doesn't matter in this context...

That is, it doesn't have to be 8 bit, because one byte
could be 7 or 9 or something. But in that case, you won't have

...which is the point, and...

uint8_t anyway (the existence of fixed-width types is optional),
so we wouldn't work anyway.

Hmm, that seems to be a valid argument.

The only thing I'd worry is that
char might be signed, in which case using it as the length of
sequence might be problematic.

I didn't explicitly mention it, but that was actually another
concern...as far as I understand the code, it doesn't actually cause a
problem at least in the Name class, but it should be safer to be
explicitly unsigned. This would make strncasecmp() unusable (unless
we convert the pointers with reinterpret_cast), but as discussed above
if we don't rely on it for the equality check this is not a problem.

comment:3 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 4

comment:4 Changed 8 years ago by jinmei

Note: I addressed the case_sensitive issue in #1603.

comment:5 Changed 8 years ago by jinmei

All of these points are now resolved or moot. I'm closing the ticket.

comment:6 Changed 8 years ago by jinmei

  • Resolution set to wontfix
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.