From b04e611b055de342abcab58e6b4ac1c80d4027a9 Mon Sep 17 00:00:00 2001 From: Francisco Maxwell Date: Thu, 24 Sep 2026 00:24:13 -0300 Subject: [PATCH] Fix IPv4 byte order handling --- clickhouse/columns/ip4.cpp | 8 ++--- clickhouse/columns/ip4.h | 2 +- clickhouse/columns/lowcardinality.cpp | 4 +-- ut/Column_ut.cpp | 2 +- ut/columns_ut.cpp | 48 +++++++++++++++++++++++---- ut/utils.cpp | 2 +- 6 files changed, 51 insertions(+), 15 deletions(-) diff --git a/clickhouse/columns/ip4.cpp b/clickhouse/columns/ip4.cpp index 78ec5bbeb..4b2eff12d 100644 --- a/clickhouse/columns/ip4.cpp +++ b/clickhouse/columns/ip4.cpp @@ -32,7 +32,7 @@ void ColumnIPv4::Append(const std::string& str) { uint32_t address; if (inet_pton(AF_INET, str.c_str(), &address) != 1) throw ValidationError("invalid IPv4 format, ip: " + str); - data_->Append(htonl(address)); + data_->Append(address); } void ColumnIPv4::Append(uint32_t ip) { @@ -40,7 +40,7 @@ void ColumnIPv4::Append(uint32_t ip) { } void ColumnIPv4::Append(in_addr ip) { - data_->Append(htonl(ip.s_addr)); + data_->Append(ip.s_addr); } void ColumnIPv4::Clear() { @@ -49,13 +49,13 @@ void ColumnIPv4::Clear() { in_addr ColumnIPv4::At(size_t n) const { in_addr addr; - addr.s_addr = ntohl(data_->At(n)); + addr.s_addr = data_->At(n); return addr; } in_addr ColumnIPv4::operator [] (size_t n) const { in_addr addr; - addr.s_addr = ntohl(data_->operator[](n)); + addr.s_addr = data_->operator[](n); return addr; } diff --git a/clickhouse/columns/ip4.h b/clickhouse/columns/ip4.h index 2253e3054..5c658ba57 100644 --- a/clickhouse/columns/ip4.h +++ b/clickhouse/columns/ip4.h @@ -27,7 +27,7 @@ class ColumnIPv4 : public Column { /// @params ip numeric value with host byte order. void Append(uint32_t ip); - /// + /// @params ip IPv4 address in network byte order. void Append(in_addr ip); /// Returns element at given row number. diff --git a/clickhouse/columns/lowcardinality.cpp b/clickhouse/columns/lowcardinality.cpp index 5a5d02982..9db98f3ea 100644 --- a/clickhouse/columns/lowcardinality.cpp +++ b/clickhouse/columns/lowcardinality.cpp @@ -233,8 +233,8 @@ inline void AppendToDictionary(Column& dictionary, const ItemView & item) { column_down_cast(dictionary).AppendRaw(item.get()); return; case Type::IPv4: - // ColumnIPv4::Append applies htonl, and GetItem returns the stored - // (already byte-swapped) value, so undo the swap to re-store as-is. + // ColumnIPv4::Append(uint32_t) accepts host byte order, while GetItem + // returns the stored network-order representation. column_down_cast(dictionary).Append(ntohl(item.get())); return; case Type::IPv6: { diff --git a/ut/Column_ut.cpp b/ut/Column_ut.cpp index df16e1701..e12115be1 100644 --- a/ut/Column_ut.cpp +++ b/ut/Column_ut.cpp @@ -313,7 +313,7 @@ inline auto convertValueForGetItem(const ColumnType& col, ValueType&& t) { || std::is_same_v) { return std::string_view{reinterpret_cast(&t), sizeof(T)}; } else if constexpr (std::is_same_v) { - return htonl(t.s_addr); + return t.s_addr; } else if constexpr (std::is_same_v) { return std::string_view(reinterpret_cast(t.s6_addr), 16); } else if constexpr (std::is_same_v) { diff --git a/ut/columns_ut.cpp b/ut/columns_ut.cpp index be8e543a7..b49be4362 100644 --- a/ut/columns_ut.cpp +++ b/ut/columns_ut.cpp @@ -954,7 +954,7 @@ TEST(ColumnsCase, ColumnIPv4) col.Append("255.255.255.255"); col.Append("127.0.0.1"); - col.Append(3585395774); + col.Append(0x3eccb4d5); col.Append(0); const in_addr ip = MakeIPv4(0x12345678); col.Append(ip); @@ -962,7 +962,7 @@ TEST(ColumnsCase, ColumnIPv4) ASSERT_EQ(5u, col.Size()); EXPECT_EQ(MakeIPv4(0xffffffff), col.At(0)); EXPECT_EQ(MakeIPv4(0x0100007f), col.At(1)); - EXPECT_EQ(MakeIPv4(3585395774), col.At(2)); + EXPECT_EQ(MakeIPv4(0xd5b4cc3e), col.At(2)); EXPECT_EQ(MakeIPv4(0), col.At(3)); EXPECT_EQ(ip, col.At(4)); @@ -976,6 +976,42 @@ TEST(ColumnsCase, ColumnIPv4) EXPECT_EQ(0u, col.Size()); } +TEST(ColumnsCase, ColumnIPv4_append_overloads_use_same_byte_order) +{ + auto from_string = ColumnIPv4(); + auto from_host_order = ColumnIPv4(); + auto from_in_addr = ColumnIPv4(); + + in_addr parsed{}; + ASSERT_EQ(1, inet_pton(AF_INET, "127.0.0.1", &parsed)); + + from_string.Append("127.0.0.1"); + from_host_order.Append(0x7f000001); + from_in_addr.Append(parsed); + + Buffer string_data; + Buffer host_order_data; + Buffer in_addr_data; + + BufferOutput string_output(&string_data); + BufferOutput host_order_output(&host_order_data); + BufferOutput in_addr_output(&in_addr_data); + + from_string.SaveBody(&string_output); + from_host_order.SaveBody(&host_order_output); + from_in_addr.SaveBody(&in_addr_output); + + string_output.Flush(); + host_order_output.Flush(); + in_addr_output.Flush(); + + EXPECT_EQ(host_order_data, string_data); + EXPECT_EQ(host_order_data, in_addr_data); + EXPECT_EQ("127.0.0.1", from_string.AsString(0)); + EXPECT_EQ("127.0.0.1", from_host_order.AsString(0)); + EXPECT_EQ("127.0.0.1", from_in_addr.AsString(0)); +} + TEST(ColumnsCase, ColumnIPv4_construct_from_data) { const auto vals = { @@ -1008,7 +1044,7 @@ TEST(ColumnsCase, ColumnIPv4_construct_from_data) EXPECT_EQ(values.size(), col.Size()); for (size_t i = 0; i < values.size(); ++i) { - EXPECT_EQ(ntohl(values[i]), col[i]) << " At pos: " << i; + EXPECT_EQ(values[i], col[i]) << " At pos: " << i; } // Make sure that `Append` and `At`/`[]` work properly @@ -1041,9 +1077,9 @@ TEST(ColumnsCase, ColumnIPv4_construct_from_rvalue_data) { }; const auto expected = { - MakeIPv4(data[0]), - MakeIPv4(data[1]), - MakeIPv4(data[2]), + MakeIPv4(htonl(data[0])), + MakeIPv4(htonl(data[1])), + MakeIPv4(htonl(data[2])), }; auto col = ColumnIPv4(std::move(data)); diff --git a/ut/utils.cpp b/ut/utils.cpp index 19cbaa10b..200230cb8 100644 --- a/ut/utils.cpp +++ b/ut/utils.cpp @@ -426,7 +426,7 @@ std::ostream& operator<<(std::ostream& ostr, const ItemView& item_view) { } case Type::IPv4: { in_addr addr; - addr.s_addr = ntohl(item_view.get()); + addr.s_addr = item_view.get(); ostr << addr; break; }