Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#4091 closed enhancement (complete)

Implement support for hex strings in client classification

Reported by: sar Owned by: fdupont
Priority: medium Milestone: Kea1.0-beta
Component: dhcp Version: git
Keywords: classification Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 8 Internal?: no

Description

Add support for hex strings, possibly as an extra constructor for TokenString?

This is listed as a medium for 1.0 but is not deemed required for 1.0

Subtickets

Change History (13)

comment:1 Changed 4 years ago by sar

  • Keywords classification added

comment:2 Changed 4 years ago by fdupont

  • Owner set to fdupont
  • Status changed from new to accepted

According to http://kea.isc.org/wiki/ClientClassificationRequirements:

E.4. Constant expression specified as hexstring (e.g. 0x54484f4d) MAY be supported. (phase 1)

Note we have a kind of hexadecimal parser for option-data, csv-format false.

comment:3 Changed 4 years ago by fdupont

Note it can't be a constructor of TokenString? because it would limit the set of possible TokenString? values.

comment:4 Changed 4 years ago by fdupont

  • Owner changed from fdupont to Unassigned
  • Status changed from accepted to reviewing

Ready for review. Note I considered the "0x" in front should be removed by the parser. This makes both the behavior and the code pretty close to option-data.

comment:5 Changed 4 years ago by fdupont

Pushed now (I remember to have pushed it but obviously it didn't work?)

comment:6 Changed 4 years ago by fdupont

  • Owner changed from Unassigned to UnAssigned

comment:7 Changed 4 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:8 follow-up: Changed 4 years ago by stephen

  • Owner changed from stephen to fdupont

Reviewed commits 97e6332fd0fd1f1da4e16c63610884dbffe28d4c through 4e42ee425f77c48c1acea2d5b50e61e39623835d

There are no unit tests for the TokenHexString class. These need to be added.

src/lib/eval/token.cc
The code assumes that the leading "0x" will be removed by the parser. I don't know if this will be the case (check with Tomek), but the code could see if the leading two characters are "0x" and, if so, ignore them.

Error: the new "evaluate" function in the token.cc file is in the class TokenString, not TokenHexString.

There is no need to check for an empty string when checking if the number of characters is odd. If the length is zero, "0 % 2" is zero, and the branch won't be taken.

As the result of "repr_.length() % 2" is supposed to be a number, ((repr_.length() % 2) != 0) is clearer than the C-style assumption that 0 is false, non-zero is true.

What happens if the hex string is empty? The only check for this condition is made when trying to decide whether to insert a leading zero. If it is empty, decodeHex() is called, the zero-length string "chars" is created and memmove called. If "chars" is zero length, the first argument to memmove - &chars[0] - may be an invalid reference. It would be better to check the string at the start of the method and bypass most of the code if it is empty.

The anticipated use is that the TokenHexString will be constructed when the configuration file is read, but the "evaluate" method called for every packet. Rather than put the conversion logic into the "evaluate" method, faster would be to perform the conversion in the constructor, store the result in the class and have the evaluate method do nothing but push the result.

comment:9 Changed 4 years ago by fdupont

Stephen, I forgot to commit and push some changes. Some of your comments should have been addressed in e20f7d8334f2416886e8e362eee6ba1e0990dc10. I'll try to address the remaining points before coming back to you.

comment:10 in reply to: ↑ 8 Changed 4 years ago by fdupont

  • Owner changed from fdupont to stephen

Replying to stephen:

Reviewed commits 97e6332fd0fd1f1da4e16c63610884dbffe28d4c through 4e42ee425f77c48c1acea2d5b50e61e39623835d

There are no unit tests for the TokenHexString class. These need to be added.

=> they were in the missing commit.

src/lib/eval/token.cc
The code assumes that the leading "0x" will be removed by the parser. I don't know if this will be the case (check with Tomek), but the code could see if the leading two characters are "0x" and, if so, ignore them.

=> I changed my mind (because of the unparse idea). Now the string argument must begin by "0x" or "0X".

Error: the new "evaluate" function in the token.cc file is in the class TokenString, not TokenHexString.

=> was fixed in the missing commit.

There is no need to check for an empty string when checking if the number of characters is odd. If the length is zero, "0 % 2" is zero, and the branch won't be taken.

=> the empty string was checked for before in the missing commit (this simplifies the code).

As the result of "repr_.length() % 2" is supposed to be a number, ((repr_.length() % 2) != 0) is clearer than the C-style assumption that 0 is false, non-zero is true.

=> Fixed in both the new code and the code it came from.

What happens if the hex string is empty? The only check for this condition is made when trying to decide whether to insert a leading zero. If it is empty, decodeHex() is called, the zero-length string "chars" is created and memmove called. If "chars" is zero length, the first argument to memmove - &chars[0] - may be an invalid reference. It would be better to check the string at the start of the method and bypass most of the code if it is empty.

=> not only I fully agree but this was exactly what the missing commit did!

The anticipated use is that the TokenHexString will be constructed when the configuration file is read, but the "evaluate" method called for every packet. Rather than put the conversion logic into the "evaluate" method, faster would be to perform the conversion in the constructor, store the result in the class and have the evaluate method do nothing but push the result.

=> I thought about decode done by the constructor, Now it is implemented.

For the ChangeLog the title of the ticket seems fine.

comment:11 Changed 4 years ago by stephen

  • Owner changed from stephen to fdupont

Reviewed commits e20f7d8334f2416886e8e362eee6ba1e0990dc10 to a496523c86e18602a729e20ad03b51d12e66368f

All OK, ready to merge. However, I would note that in token.cc, if you change

    // Check "0x" or "0x" in front
    if ((str.size() < 2) ||

to

    // Check string starts "0x" or "0x" and has at least one additional character.
    if ((str.size() < 3) ||

it will be possible to remove the check

    // Eliminate the no digit case first
    if (digits.empty()) {
        return;
    }

(If you decide to make that change before merging, I don't need to review this again.)

comment:12 Changed 4 years ago by fdupont

  • Resolution set to complete
  • Status changed from reviewing to closed

0[xX]{hexdigit}* vs 0[xX]{hexdigit}+ was a point where I had no strong opinion.

Merged, closing.

comment:13 Changed 4 years ago by tomek

  • Milestone changed from Kea1.0 to Kea1.0-beta

Milestone renamed

Note: See TracTickets for help on using tickets.