Skip to content

Commit 1784a59

Browse files
CaellianCaellian
committed
Fix x11_change_property not properly copying data
Signed-off-by: Tin Švagelj <tin.svagelj@live.com>
1 parent 384822e commit 1784a59

File tree

5 files changed

+146
-84
lines changed

5 files changed

+146
-84
lines changed

‎src/x11.cc

+1-1
Original file line numberDiff line numberDiff line change
@@ -1146,7 +1146,7 @@ void set_struts(alignment align) {
11461146

11471147
Atom strut = ATOM(_NET_WM_STRUT);
11481148
if (strut != None) {
1149-
long sizes[STRUT_COUNT] = {0};
1149+
long sizes[STRUT_COUNT] = {};
11501150

11511151
int display_width = workarea.width();
11521152
int display_height = workarea.height();

‎tests/CMakeLists.txt

+1-3
Original file line numberDiff line numberDiff line change
@@ -25,10 +25,8 @@ excluding_any("wayland" IF NOT BUILD_WAYLAND)
2525

2626
# Mocking works because it's linked before conky_core, so the linker uses mock
2727
# implementations instead of those that are linked later.
28-
add_library(Catch2 STATIC catch2/catch_amalgamated.cpp)
29-
3028
add_library(conky-mock OBJECT ${mock_sources})
31-
target_link_libraries(conky-mock Catch2)
29+
add_library(Catch2 STATIC catch2/catch_amalgamated.cpp)
3230

3331
add_executable(test-conky test-common.cc ${test_sources})
3432
target_include_directories(test-conky

‎tests/mock/x11-mock.hh

+105-73
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,11 @@
55
#include <X11/Xatom.h>
66
#include <X11/Xlib.h>
77
#include <X11/Xutil.h>
8+
#include <cstddef>
89
#include <cstdint>
910
#include <string>
1011
#include <string_view>
12+
#include <vector>
1113

1214
#include "mock.hh"
1315

@@ -36,6 +38,9 @@ enum x11_property_type {
3638

3739
Atom name_to_atom(const char *name);
3840
const std::string_view atom_to_name(Atom atom);
41+
size_t format_size(std::size_t format);
42+
void dump_x11_blob(const std::byte *data, std::size_t format,
43+
std::size_t length);
3944

4045
/// Mutation produced by creating new `Atom`s.
4146
struct x11_define_atom : public state_change {
@@ -62,29 +67,29 @@ class x11_change_property : public state_change {
6267
Atom m_type;
6368
std::size_t m_format;
6469
set_property_mode m_mode;
65-
const unsigned char *m_data;
70+
std::vector<std::byte> m_data;
6671
std::size_t m_element_count;
6772

6873
public:
6974
x11_change_property(Atom property, Atom type, std::size_t format,
70-
set_property_mode mode, const unsigned char *data,
75+
set_property_mode mode, const std::byte *data,
7176
std::size_t element_count)
7277
: m_property(property),
7378
m_type(type),
7479
m_format(format),
7580
m_mode(mode),
76-
m_data(data),
77-
m_element_count(element_count) {}
81+
m_element_count(element_count),
82+
m_data(std::vector(data, data + format_size(format) * element_count)) {}
7883

7984
static std::string change_name() { return "x11_change_property"; }
8085

81-
Atom property() { return m_property; }
82-
std::string_view property_name() { return atom_to_name(m_property); }
83-
Atom type() { return m_type; }
84-
std::string_view type_name() { return atom_to_name(m_type); }
85-
std::size_t format() { return m_format; }
86-
set_property_mode mode() { return m_mode; }
87-
std::string_view mode_name() {
86+
Atom property() const { return m_property; }
87+
std::string_view property_name() const { return atom_to_name(m_property); }
88+
Atom type() const { return m_type; }
89+
std::string_view type_name() const { return atom_to_name(m_type); }
90+
std::size_t format() const { return m_format; }
91+
set_property_mode mode() const { return m_mode; }
92+
std::string_view mode_name() const {
8893
switch (m_mode) {
8994
case mock::set_property_mode::REPLACE:
9095
return "replace";
@@ -96,8 +101,8 @@ class x11_change_property : public state_change {
96101
return "other";
97102
}
98103
}
99-
std::size_t element_count() { return m_element_count; }
100-
const unsigned char *const data() { return m_data; }
104+
std::size_t element_count() const { return m_element_count; }
105+
const std::byte *data() const { return m_data.data(); }
101106

102107
std::string debug() {
103108
return debug_format(
@@ -107,26 +112,24 @@ class x11_change_property : public state_change {
107112
m_element_count);
108113
}
109114
};
110-
111-
template <class T>
112-
struct always_false : std::false_type {};
113115
} // namespace mock
114116

117+
#define REQUIRE_FORMAT_SIZE(format, T) REQUIRE(format == (sizeof(T) * 8))
118+
115119
// These are only macros because including Catch2 from mocking causes spurious
116-
// errors.
120+
// errors. I whish they weren't because they're such a pain to write this way.
117121

118122
// Originally a single templated function:
119123
//
120124
// template <typename D, const std::size_t Count>
121125
// const D &expect_x11_data(
122-
// const unsigned char * const data, Atom type, std::size_t format,
126+
// const std::byte* data, Atom type, std::size_t format,
123127
// std::size_t element_count
124128
// ) {...}
125129
//
126130
// It is a somewhat large blob, but most of it will be compiled away. The only
127131
// downside is that lambdas must return owned values.
128132

129-
#define REQUIRE_FORMAT_SIZE(format, T) REQUIRE(format == (sizeof(T) * 8))
130133
#define EXPECT_X11_VALUE(data, type, format, element_count, T) \
131134
[]() { \
132135
if constexpr (std::is_same_v<XID, std::uint32_t> && \
@@ -225,6 +228,21 @@ struct always_false : std::false_type {};
225228
} \
226229
}()
227230

231+
#define _COPY_C_ARRAY_TO_CAST(BaseT, TargetT, Length, source) \
232+
[&]() { \
233+
auto values = reinterpret_cast<const BaseT *>(source); \
234+
auto result = std::array<TargetT, Length>{}; \
235+
for (size_t i = 0; i < Length; i++) { \
236+
if constexpr (std::numeric_limits<BaseT>::max() > \
237+
std::numeric_limits<TargetT>::max()) { \
238+
CHECK(values[i] >= std::numeric_limits<TargetT>::min()); \
239+
CHECK(values[i] <= std::numeric_limits<TargetT>::max()); \
240+
} \
241+
result[i] = static_cast<TargetT>(values[i]); \
242+
} \
243+
return result; \
244+
}()
245+
228246
#define EXPECT_X11_ARRAY(data, type, format, element_count, T, Count) \
229247
[&]() { \
230248
if constexpr (std::is_same_v<XID, std::uint32_t> && \
@@ -244,14 +262,14 @@ struct always_false : std::false_type {};
244262
} \
245263
REQUIRE_FORMAT_SIZE(format, std::uint32_t); \
246264
REQUIRE(element_count == Count); \
247-
return *reinterpret_cast<const std::array<T, Count> *>(data); \
265+
return _COPY_C_ARRAY_TO_CAST(long, std::uint32_t, Count, data); \
248266
} else if constexpr (std::is_same_v<T, std::uint32_t>) { \
249267
if (type != mock::x11_property_type::CARDINAL) { \
250268
FAIL("expected CARDINAL array; got: " << mock::atom_to_name(type)); \
251269
} \
252270
REQUIRE_FORMAT_SIZE(format, std::uint32_t); \
253271
REQUIRE(element_count == Count); \
254-
return *reinterpret_cast<const std::array<T, Count> *>(data); \
272+
return _COPY_C_ARRAY_TO_CAST(long, std::uint32_t, Count, data); \
255273
} else if constexpr (std::is_same_v<T, XID>) { \
256274
if (!(type == mock::x11_property_type::ATOM || \
257275
type == mock::x11_property_type::BITMAP || \
@@ -266,68 +284,82 @@ struct always_false : std::false_type {};
266284
} \
267285
REQUIRE_FORMAT_SIZE(format, XID); \
268286
REQUIRE(element_count == Count); \
269-
return *reinterpret_cast<const std::array<T, Count> *>(data); \
287+
return _COPY_C_ARRAY_TO_CAST(long, T, Count, data); \
270288
} else if constexpr (std::is_same_v<T, std::int32_t>) { \
271289
if (type != mock::x11_property_type::INTEGER) { \
272290
FAIL("expected INTEGER array; got: " << mock::atom_to_name(type)); \
273291
} \
274-
REQUIRE_FORMAT_SIZE(format, std::uint32_t); \
292+
REQUIRE_FORMAT_SIZE(format, std::int32_t); \
275293
REQUIRE(element_count == Count); \
276-
return *reinterpret_cast<const std::array<T, Count> *>(data); \
294+
return _COPY_C_ARRAY_TO_CAST(long, std::int32_t, Count, data); \
277295
} else { \
278296
throw "unimplemented conversion"; \
279297
} \
280298
}()
281299

282-
#define EXPECT_X11_VEC(data, type, format, element_count, T) \
283-
[&]() { \
284-
REQUIRE(sizeof(T) == format); \
285-
if constexpr (std::is_same_v<XID, std::uint32_t> && \
286-
std::is_same_v<T, std::uint32_t>) { \
287-
if (!(type == mock::x11_property_type::ATOM || \
288-
type == mock::x11_property_type::BITMAP || \
289-
type == mock::x11_property_type::CARDINAL || \
290-
type == mock::x11_property_type::PIXMAP || \
291-
type == mock::x11_property_type::COLORMAP || \
292-
type == mock::x11_property_type::CURSOR || \
293-
type == mock::x11_property_type::DRAWABLE || \
294-
type == mock::x11_property_type::FONT || \
295-
type == mock::x11_property_type::VISUALID || \
296-
type == mock::x11_property_type::WINDOW)) { \
297-
FAIL("expected unsigned long array; got: " \
298-
<< mock::atom_to_name(type)); \
299-
} \
300-
REQUIRE_FORMAT_SIZE(format, std::uint32_t); \
301-
return std::vector<reinterpret_cast<const T *>(data), element_count>; \
302-
} else if constexpr (std::is_same_v<T, std::uint32_t>) { \
303-
if (type != mock::x11_property_type::CARDINAL) { \
304-
FAIL("expected CARDINAL array; got: " << mock::atom_to_name(type)); \
305-
} \
306-
REQUIRE_FORMAT_SIZE(format, std::uint32_t); \
307-
return std::vector<reinterpret_cast<const T *>(data), element_count>; \
308-
} else if constexpr (std::is_same_v<T, XID>) { \
309-
if (!(type == mock::x11_property_type::ATOM || \
310-
type == mock::x11_property_type::BITMAP || \
311-
type == mock::x11_property_type::PIXMAP || \
312-
type == mock::x11_property_type::COLORMAP || \
313-
type == mock::x11_property_type::CURSOR || \
314-
type == mock::x11_property_type::DRAWABLE || \
315-
type == mock::x11_property_type::FONT || \
316-
type == mock::x11_property_type::VISUALID || \
317-
type == mock::x11_property_type::WINDOW)) { \
318-
FAIL("expected XID data; got: " << mock::atom_to_name(type)); \
319-
} \
320-
REQUIRE_FORMAT_SIZE(format, XID); \
321-
return std::vector<reinterpret_cast<const T *>(data), element_count>; \
322-
} else if constexpr (std::is_same_v<T, std::int32_t>) { \
323-
if (type != mock::x11_property_type::INTEGER) { \
324-
FAIL("expected INTEGER array; got: " << mock::atom_to_name(type)); \
325-
} \
326-
REQUIRE_FORMAT_SIZE(format, std::int32_t); \
327-
return std::vector<reinterpret_cast<const T *>(data), element_count>; \
328-
} else { \
329-
throw "unimplemented conversion"; \
330-
} \
300+
#define _COPY_C_ARRAY_TO_VEC(BaseT, TargetT, source, length) \
301+
[&]() { \
302+
auto values = reinterpret_cast<const BaseT *>(source); \
303+
auto result = std::vector<TargetT>(length); \
304+
for (const BaseT *it = values; it < values + length; it++) { \
305+
if constexpr (std::numeric_limits<BaseT>::max() > \
306+
std::numeric_limits<TargetT>::max()) { \
307+
CHECK(*it >= std::numeric_limits<TargetT>::min()); \
308+
CHECK(*it <= std::numeric_limits<TargetT>::max()); \
309+
} \
310+
result.push_back(*it); \
311+
} \
312+
return result; \
313+
}()
314+
315+
#define EXPECT_X11_VEC(data, type, format, element_count, T) \
316+
[&]() { \
317+
if constexpr (std::is_same_v<XID, std::uint32_t> && \
318+
std::is_same_v<T, std::uint32_t>) { \
319+
if (!(type == mock::x11_property_type::ATOM || \
320+
type == mock::x11_property_type::BITMAP || \
321+
type == mock::x11_property_type::CARDINAL || \
322+
type == mock::x11_property_type::PIXMAP || \
323+
type == mock::x11_property_type::COLORMAP || \
324+
type == mock::x11_property_type::CURSOR || \
325+
type == mock::x11_property_type::DRAWABLE || \
326+
type == mock::x11_property_type::FONT || \
327+
type == mock::x11_property_type::VISUALID || \
328+
type == mock::x11_property_type::WINDOW)) { \
329+
FAIL("expected unsigned long array; got: " \
330+
<< mock::atom_to_name(type)); \
331+
} \
332+
REQUIRE_FORMAT_SIZE(format, std::uint32_t); \
333+
return _COPY_C_ARRAY_TO_VEC(long, std::uint32_t, data, element_count); \
334+
} else if constexpr (std::is_same_v<T, std::uint32_t>) { \
335+
if (type != mock::x11_property_type::CARDINAL) { \
336+
FAIL("expected CARDINAL array; got: " << mock::atom_to_name(type)); \
337+
} \
338+
REQUIRE_FORMAT_SIZE(format, std::uint32_t); \
339+
return _COPY_C_ARRAY_TO_VEC(long, std::uint32_t, data, element_count); \
340+
} else if constexpr (std::is_same_v<T, XID>) { \
341+
if (!(type == mock::x11_property_type::ATOM || \
342+
type == mock::x11_property_type::BITMAP || \
343+
type == mock::x11_property_type::PIXMAP || \
344+
type == mock::x11_property_type::COLORMAP || \
345+
type == mock::x11_property_type::CURSOR || \
346+
type == mock::x11_property_type::DRAWABLE || \
347+
type == mock::x11_property_type::FONT || \
348+
type == mock::x11_property_type::VISUALID || \
349+
type == mock::x11_property_type::WINDOW)) { \
350+
FAIL("expected XID data; got: " << mock::atom_to_name(type)); \
351+
} \
352+
REQUIRE_FORMAT_SIZE(format, XID); \
353+
return _COPY_C_ARRAY_TO_VEC(long, XID, data, element_count); \
354+
} else if constexpr (std::is_same_v<T, std::int32_t>) { \
355+
if (type != mock::x11_property_type::INTEGER) { \
356+
FAIL("expected INTEGER array; got: " << mock::atom_to_name(type)); \
357+
} \
358+
REQUIRE_FORMAT_SIZE(format, std::int32_t); \
359+
return _COPY_C_ARRAY_TO_VEC(long, std::int32_t, data, element_count); \
360+
} else { \
361+
throw "unimplemented conversion"; \
362+
} \
331363
}()
332364

333365
#endif /* X11_MOCK_HH */

‎tests/mock/x11.cc

+32-2
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
#include <array>
22
#include <cstdint>
3+
#include <cstdio>
34
#include <cstring>
45
#include <memory>
56
#include <string>
@@ -111,6 +112,33 @@ const std::string_view atom_to_name(Atom atom) {
111112
}
112113
return "UNKNOWN";
113114
}
115+
116+
size_t format_size(std::size_t format) {
117+
if (format == 32) {
118+
return sizeof(long);
119+
} else if (format == 16) {
120+
return sizeof(short);
121+
} else if (format == 8) {
122+
return sizeof(char);
123+
} else {
124+
throw "invalid format";
125+
}
126+
}
127+
void dump_x11_blob(const std::byte *data, std::size_t format,
128+
std::size_t length) {
129+
size_t entry_len = format_size(format);
130+
for (size_t i = 0; i < length * entry_len; i++) {
131+
if (((i + 1) % entry_len) == 1) { printf("%p: ", data + i); }
132+
// Print bytes in order:
133+
// printf("%02x ", data[i]);
134+
// Reorder bytes:
135+
printf("%02x ", (unsigned char)data[((i / entry_len - 1) * entry_len) +
136+
(2 * entry_len - 1 - (i % entry_len))]);
137+
if (i > 0 && ((i + 1) % entry_len) == 0) { puts(""); }
138+
}
139+
printf("Total bytes: %d\n", (int)(length * entry_len));
140+
puts("");
141+
}
114142
} // namespace mock
115143

116144
extern "C" {
@@ -130,9 +158,11 @@ Atom XInternAtom(Display *display, const char *atom_name, int only_if_exists) {
130158
int XChangeProperty(Display *display, Window w, Atom property, Atom type,
131159
int format, int mode, const unsigned char *data,
132160
int nelements) {
161+
// printf("Setting %s property data:\n", mock::atom_to_name(property).data());
162+
// dump_x11_blob((const std::byte *)data, format, nelements);
133163
mock::push_state_change(std::make_unique<mock::x11_change_property>(
134-
property, type, format, static_cast<mock::set_property_mode>(mode), data,
135-
nelements));
164+
property, type, format, static_cast<mock::set_property_mode>(mode),
165+
(const std::byte *)data, nelements));
136166
return Success;
137167
}
138168
}

0 commit comments

Comments
 (0)