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

(MODULES-2409) Endian-ness incorrect for DWORD and QWORD #95

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 4 additions & 24 deletions lib/puppet/provider/registry_value/registry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -174,28 +174,6 @@ def write_value
end
end

BITS_PER_BYTE = 8
BYTE_MASK = 0xFF

def to_byte_array(num, of_length)
bytes = []
while
# mask only least significant byte and prepend
bytes.unshift(num & BYTE_MASK)

# nothing left to shave off
break if (num <= BYTE_MASK)

# shift off the least significant byte and continue
num >>= BITS_PER_BYTE
end

pad = of_length - bytes.length
bytes.concat([0] * pad) if pad > 0

bytes
end

def data_to_bytes(type, data)
bytes = []

Expand All @@ -210,9 +188,11 @@ def data_to_bytes(type, data)
when Win32::Registry::REG_BINARY
bytes = data.bytes.to_a
when Win32::Registry::REG_DWORD
bytes = to_byte_array(data, FFI::Type::UINT32.size)
# L is 32-bit unsigned native (little) endian order
bytes = [data].pack('L').unpack('C*')
when Win32::Registry::REG_QWORD
bytes = to_byte_array(data, FFI::Type::UINT64.size)
# Q is 64-bit unsigned native (little) endian order
bytes = [data].pack('Q').unpack('C*')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

irb(main):007:0> [0x12345678].pack('L').unpack('C*').map {|ch| ch.to_s(16)}
=> ["78", "56", "34", "12"]
irb(main):008:0> [0x1234567890ABCDEF].pack('Q').unpack('C*').map {|ch| ch.to_s(16)}
=> ["ef", "cd", "ab", "90", "78", "56", "34", "12"]

👍

else
raise TypeError, "Unsupported type #{type}"
end
Expand Down
46 changes: 45 additions & 1 deletion spec/unit/puppet/provider/registry_value_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@
# also, expect that we're not using Rubys each_key / each_value which exhibit bad behavior
Win32::Registry.any_instance.expects(:each_key).never
Win32::Registry.any_instance.expects(:each_value).never

# this covers []= write_s write_i and write_bin
Win32::Registry.any_instance.expects(:write).never
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

end
end

Expand Down Expand Up @@ -93,6 +96,7 @@ def create_and_destroy(path, reg_type, data)

reg_value.provider.create
reg_value.provider.exists?.should be_true
expect(reg_value.provider.data).to eq([data].flatten)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have tests to verifying dword & qword little endian behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not explicitly have such tests.

However, as long as the randomly generated values here are above 0xFF, we automatically test endian-ness as a side effect. If it's wrong, we will have test failures here.

I also thought about adding acceptance tests that have hardcoded values that are known to trigger endianness flubs, but fixing the current manifest to do that instead of injecting "phase" is not currently straightforward.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests added

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Crazy question. Do we have tests now verifying every type that can be written?


reg_value.provider.destroy
reg_value.provider.exists?.should be_false
Expand All @@ -107,7 +111,7 @@ def create_and_destroy(path, reg_type, data)
end

it "can destroy a randomly created REG_BINARY value" do
create_and_destroy(path, :binary, '01011010')
create_and_destroy(path, :binary, '01 01 10 10')
end

it "can destroy a randomly created REG_DWORD value" do
Expand All @@ -123,6 +127,46 @@ def create_and_destroy(path, reg_type, data)
end
end

context "when writing numeric values" do
let (:path) { path = "hklm\\#{puppet_key}\\#{subkey_name}\\#{SecureRandom.uuid}" }

after(:each) do
reg_value = type.new(:path => path, :provider => described_class.name)

reg_value.provider.destroy
expect(reg_value.provider).to_not be_exists
end

def write_and_read_value(path, reg_type, value)
reg_value = type.new(:path => path,
:type => reg_type,
:data => value,
:provider => described_class.name)

reg_value.provider.create
expect(reg_value.provider).to be_exists
expect(reg_value.provider.type).to eq(reg_type)

written = reg_value.provider.data.first
expect(written).to eq(value)
end


# values chosen at 1 bit past previous byte boundary
[0xFF + 1, 0xFFFF + 1, 0xFFFFFF + 1, 0xFFFFFFFF].each do |value|
it "properly round-trips written values by converting endianness properly" do
write_and_read_value(path, :dword, value)
write_and_read_value(path, :qword, value)
end
end

[0xFFFFFFFFFF + 1, 0xFFFFFFFFFFFF + 1, 0xFFFFFFFFFFFFFF + 1, 0xFFFFFFFFFFFFFFFF].each do |value|
it "properly round-trips written values by converting endianness properly" do
write_and_read_value(path, :qword, value)
end
end
end

context "when reading non-ASCII values" do
ENDASH_UTF_8 = [0xE2, 0x80, 0x93]
ENDASH_UTF_16 = [0x2013]
Expand Down