Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement support for arbitrary separator in stdnse tohex() #2901

Closed
wants to merge 1 commit into from

Conversation

nnposter
Copy link

@nnposter nnposter commented Aug 5, 2024

Function tohex() in stdnse is documented to support arbitrary separator for groups of hex digits. However, the actual code uses hard-coded colon as the separator, ignoring the corresponding input option. One manifestation of this gap is #2744 . This PR aligns the tohex() implementation with the documentation.

As a side benefit, the function is now significantly faster, particularly with larger inputs. With the default group size of 2, the improvement is 2.6x for strings of 4 characters, 9.9x for 16, 39.3x for 64, and 122.5x for 256.

The PR will be committed after August 25, 2024, unless concerns are raised.

@dmiller-nmap
Copy link

Looks great! I would only suggest simplifying the calculations and use of the extra variable:

  if separator then
    local group = options.group or 2
    -- If exactly a multiple of group size, there will be a trailing separator
    local extra = #hex % group
    hex = gsub(hex, rep(".", group), "%0" .. gsub(separator, "%%", "%%%%"))
    if extra == 0 then
      hex = sub(hex, 1, -(#separator+1))
    end
  end

This is easier to read and understand for me, though I didn't see a measurable performance difference to your code. Good catch escaping the (admittedly unlikely) % separator.

@nnposter
Copy link
Author

nnposter commented Aug 5, 2024

I believe that this simplification would change group alignment of the output, which might or might not be OK. The effort to preserve the existing output faithfully is the reason why my code is unquestionably more complex.

As an example, for tohex(0x123, {separator=':'}), the legacy code and my PR produce "1:23", but the simplification produces "12:3".

@dmiller-nmap Opinions?

@nmap-bot nmap-bot closed this in c661b0a Aug 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants