Skip to content

Commit 31a4bba

Browse files
tomerdbzgalnoam
authored andcommitted
issue: 4561820 Fix type validation for integers
Integer configuration parameters were incorrectly accepting boolean and string values instead of throwing type validation errors. This fix adds proper type checking in parameter_descriptor::get_value() to ensure type safety. Changes: - Add type validation check to reject mismatched types - Remove deprecated kernel_fd_attention_level parameter from schema - Update tests to verify type validation works correctly for all types - Add comprehensive unit tests for type mismatch scenarios Fixes issue where these integer parameters accepted invalid types: - performance.rings.tx.udp_buffer_batch - performance.polling.kernel_fd_attention_level (now removed) - performance.buffers.tx.global_array_size The fix ensures that boolean and string values are properly rejected for integer parameters, causing configuration loading to fail with appropriate error messages instead of silently accepting invalid values. Signed-off-by: Tomer Cabouly <[email protected]>
1 parent 2ac62e0 commit 31a4bba

File tree

8 files changed

+289
-34
lines changed

8 files changed

+289
-34
lines changed

README

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -666,10 +666,6 @@ performance.polling.iomux.skip_os
666666
For select() or poll() this will force XLIO to check the non offloaded fd even though an offloaded socket has ready packets found while polling. Maps to **XLIO_SELECT_SKIP_OS** environment variable.
667667
Default value is 4
668668

669-
performance.polling.kernel_fd_attention_level
670-
Controls threshold for checking kernel file descriptors during polling. 0 means never check. Maps to **XLIO_RING_KERNEL_FD_ATTENTION_LEVEL** environment variable. This setting affects how often XLIO checks for activity on non-offloaded kernel file descriptors while processing offloaded sockets.
671-
Default value is 10
672-
673669
performance.polling.max_rx_poll_batch
674670
Maximum number of receive buffers processed in a single poll operation. Maps to **XLIO_CQ_POLL_BATCH_MAX** environment variable. Max size of the array while polling the CQs in the XLIO.
675671
Default value is 16

src/core/config/descriptor_providers/xlio_config_schema.json

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1026,13 +1026,6 @@
10261026
"title": "Offload transition poll count",
10271027
"description": "XLIO maps all UDP sockets as potential offloaded capable. Only after the ADD_MEMBERSHIP does the offload start to work and the CQ polling kicks in XLIO. This parameter controls the polling count during this transition phase where the socket is a UDP unicast socket and no multicast addresses were added to it. Once the first ADD_MEMBERSHIP is called the RX poll duration setting takes effect. Value range is similar to the RX poll duration; -1 means infinite, 0 disables. Maps to XLIO_RX_POLL_INIT environment variable."
10281028
},
1029-
"kernel_fd_attention_level": {
1030-
"type": "integer",
1031-
"minimum": 0,
1032-
"default": 10,
1033-
"title": "Kernel FD attention threshold",
1034-
"description": "Controls threshold for checking kernel file descriptors during polling. 0 means never check. Maps to XLIO_RING_KERNEL_FD_ATTENTION_LEVEL environment variable. This setting affects how often XLIO checks for activity on non-offloaded kernel file descriptors while processing offloaded sockets."
1035-
},
10361029
"max_rx_poll_batch": {
10371030
"type": "integer",
10381031
"minimum": 0,

src/core/config/descriptors/parameter_descriptor.cpp

Lines changed: 59 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -179,32 +179,74 @@ void parameter_descriptor::add_constraint(constraint_t constraint)
179179
m_constraints.push_back(std::move(constraint));
180180
}
181181

182-
std::experimental::any parameter_descriptor::get_value(const std::experimental::any &val) const
182+
std::experimental::any parameter_descriptor::convert_string_to_int64(const std::string &val) const
183183
{
184-
std::experimental::any result = val;
185-
186-
// First, apply string mappings if applicable
187-
if (result.type() == typeid(std::string)) {
188-
auto it = m_string_mapping.find(std::experimental::any_cast<std::string>(result));
189-
if (it != m_string_mapping.end()) {
190-
result = it->second;
184+
// Try value transformer first (e.g., "1GB" -> 1073741824)
185+
if (m_value_transformer) {
186+
std::experimental::any result = m_value_transformer(std::experimental::any(val));
187+
if (std::type_index(result.type()) != m_type) {
188+
throw std::experimental::bad_any_cast();
191189
}
190+
return result;
192191
}
193192

194-
// Then apply value transformer if available
195-
if (m_value_transformer) {
196-
result = m_value_transformer(result);
193+
// Try string mapping (e.g., "enabled" -> 1)
194+
auto it = m_string_mapping.find(val);
195+
if (it != m_string_mapping.end()) {
196+
return it->second;
197+
}
198+
199+
throw std::experimental::bad_any_cast();
200+
}
201+
202+
std::experimental::any parameter_descriptor::get_value(bool val) const
203+
{
204+
// For boolean values, no string mapping or transformation is typically needed
205+
// Just validate that the parameter type is boolean
206+
if (m_type != typeid(bool)) {
207+
throw std::experimental::bad_any_cast();
208+
}
209+
210+
return std::experimental::any(val);
211+
}
212+
213+
std::experimental::any parameter_descriptor::get_value(const std::string &val) const
214+
{
215+
// Case 1: Parameter expects string - direct pass-through
216+
if (m_type == typeid(std::string)) {
217+
return std::experimental::any(val);
218+
}
219+
220+
// Case 2: Parameter expects int64_t - convert string via transformer or mapping
221+
if (m_type == typeid(int64_t)) {
222+
return convert_string_to_int64(val);
197223
}
198224

199-
// Validate type compatibility for string mappings
200-
if (!m_string_mapping.empty() && result.type() != typeid(int64_t) &&
201-
result.type() != typeid(std::string)) {
202-
// If there is a string mapping, then the value must be a string or an int as it represents
203-
// an enum
225+
// Case 3: Unsupported type conversion
226+
throw std::experimental::bad_any_cast();
227+
}
228+
229+
std::experimental::any parameter_descriptor::get_value(int64_t val) const
230+
{
231+
if (m_type != typeid(int64_t)) {
204232
throw std::experimental::bad_any_cast();
205233
}
234+
return std::experimental::any(val);
235+
}
206236

207-
return result;
237+
std::experimental::any parameter_descriptor::get_value(const std::experimental::any &val) const
238+
{
239+
// Dispatch to type-specific convenience methods based on the input type
240+
if (val.type() == typeid(bool)) {
241+
return get_value(std::experimental::any_cast<bool>(val));
242+
} else if (val.type() == typeid(std::string)) {
243+
return get_value(std::experimental::any_cast<std::string>(val));
244+
} else if (val.type() == typeid(int64_t)) {
245+
return get_value(std::experimental::any_cast<int64_t>(val));
246+
} else {
247+
// For unsupported types, throw an exception
248+
throw std::experimental::bad_any_cast();
249+
}
208250
}
209251

210252
void parameter_descriptor::validate_constraints(const std::experimental::any &value) const

src/core/config/descriptors/parameter_descriptor.h

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,28 @@ class parameter_descriptor {
110110
*/
111111
std::experimental::any get_value(const std::experimental::any &val) const;
112112

113+
/**
114+
* @brief Type-specific get_value for boolean values
115+
* @param val Boolean input value
116+
* @return Resolved boolean value as std::experimental::any
117+
*/
118+
std::experimental::any get_value(bool val) const;
119+
120+
/**
121+
* @brief Type-specific get_value for string values
122+
* @param val String input value
123+
* @return Resolved value (mapped and transformed value if applicable, original otherwise) as
124+
* std::experimental::any
125+
*/
126+
std::experimental::any get_value(const std::string &val) const;
127+
128+
/**
129+
* @brief Type-specific get_value for int64_t values
130+
* @param val int64_t input value
131+
* @return Resolved int64_t value as std::experimental::any
132+
*/
133+
std::experimental::any get_value(int64_t val) const;
134+
113135
/**
114136
* @brief Gets the type of the parameter
115137
* @return The type of the parameter
@@ -125,6 +147,21 @@ class parameter_descriptor {
125147
private:
126148
static int64_t parse_memory_size(const char *str);
127149

150+
/**
151+
* @brief Check if the given type matches the expected parameter type
152+
* @param type The type to check
153+
* @return True if the type matches the expected parameter type
154+
*/
155+
bool is_expected_type(const std::type_index &type) const;
156+
157+
/**
158+
* @brief Convert a string value to int64_t using transformer or string mapping
159+
* @param val The string value to convert
160+
* @return The converted int64_t value as std::experimental::any
161+
* @throws std::experimental::bad_any_cast if conversion fails
162+
*/
163+
std::experimental::any convert_string_to_int64(const std::string &val) const;
164+
128165
std::experimental::any m_default_value; /**< Default parameter value */
129166
std::vector<constraint_t> m_constraints; /**< Validation constraints */
130167
std::map<std::string, std::experimental::any> m_string_mapping; /**< String-to-value mappings */

src/core/config/mappings.py

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,6 @@
8585
"performance.polling.iomux.poll_os_ratio": "XLIO_SELECT_POLL_OS_RATIO",
8686
"performance.polling.iomux.poll_usec": "XLIO_SELECT_POLL",
8787
"performance.polling.iomux.skip_os": "XLIO_SELECT_SKIP_OS",
88-
"performance.polling.kernel_fd_attention_level": "XLIO_RING_KERNEL_FD_ATTENTION_LEVEL",
8988
"performance.polling.max_rx_poll_batch": "XLIO_CQ_POLL_BATCH_MAX",
9089
"performance.polling.nonblocking_eagain": "XLIO_TX_NONBLOCKED_EAGAINS",
9190
"performance.polling.offload_transition_poll_count": "XLIO_RX_POLL_INIT",

tests/unit_tests/config/config_registry.cpp

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -504,3 +504,131 @@ TEST(config, config_registry_memory_size_transformer_inline_config)
504504
// 2GB = 2 * 1024 * 1024 * 1024 = 2147483648 bytes
505505
ASSERT_EQ(2147483648LL, registry.get_value<int64_t>("core.memory_limit"));
506506
}
507+
508+
// Helper descriptor for type validation tests
509+
static const char *sample_type_validation_descriptor = R"({
510+
"$schema": "http://json-schema.org/draft-07/schema#",
511+
"title": "XLIO Configuration Schema",
512+
"description": "Schema for XLIO configuration",
513+
"type": "object",
514+
"properties": {
515+
"performance": {
516+
"type": "object",
517+
"description": "Performance-related settings",
518+
"properties": {
519+
"rings": {
520+
"type": "object",
521+
"description": "Ring configuration",
522+
"properties": {
523+
"tx": {
524+
"type": "object",
525+
"description": "Transmission ring settings",
526+
"properties": {
527+
"udp_buffer_batch": {
528+
"type": "integer",
529+
"default": 16,
530+
"minimum": 1,
531+
"title": "TX buffer batch size",
532+
"description": "Number of TX buffers fetched by a UDP socket at once"
533+
}
534+
}
535+
}
536+
}
537+
}
538+
}
539+
}
540+
}
541+
})";
542+
543+
TEST(config, config_registry_boolean_type_mismatch_throws)
544+
{
545+
// Test that boolean values are rejected for integer parameters during loading
546+
// This test specifically catches the bug where boolean values were accepted for integer
547+
// parameters
548+
549+
conf_file_writer json_config(R"({
550+
"performance": {
551+
"rings": {
552+
"tx": {
553+
"udp_buffer_batch": true
554+
}
555+
}
556+
}
557+
})");
558+
559+
std::queue<std::unique_ptr<loader>> loaders;
560+
loaders.push(std::make_unique<json_loader>(json_config.get()));
561+
562+
ASSERT_THROW(config_registry(
563+
std::move(loaders),
564+
std::make_unique<json_descriptor_provider>(sample_type_validation_descriptor)),
565+
xlio_exception);
566+
}
567+
568+
TEST(config, config_registry_string_type_mismatch_throws)
569+
{
570+
// Test that string values are rejected for integer parameters during loading
571+
572+
conf_file_writer json_config(R"({
573+
"performance": {
574+
"rings": {
575+
"tx": {
576+
"udp_buffer_batch": "invalid"
577+
}
578+
}
579+
}
580+
})");
581+
582+
std::queue<std::unique_ptr<loader>> loaders;
583+
loaders.push(std::make_unique<json_loader>(json_config.get()));
584+
585+
ASSERT_THROW(config_registry(
586+
std::move(loaders),
587+
std::make_unique<json_descriptor_provider>(sample_type_validation_descriptor)),
588+
xlio_exception);
589+
}
590+
591+
TEST(config, config_registry_valid_integer_works)
592+
{
593+
// Test that valid integer values work correctly
594+
conf_file_writer json_config(R"({
595+
"performance": {
596+
"rings": {
597+
"tx": {
598+
"udp_buffer_batch": 32
599+
}
600+
}
601+
}
602+
})");
603+
604+
std::queue<std::unique_ptr<loader>> loaders;
605+
loaders.push(std::make_unique<json_loader>(json_config.get()));
606+
607+
config_registry registry(
608+
std::move(loaders),
609+
std::make_unique<json_descriptor_provider>(sample_type_validation_descriptor));
610+
611+
ASSERT_EQ(32LL, registry.get_value<int64_t>("performance.rings.tx.udp_buffer_batch"));
612+
}
613+
614+
TEST(config, config_registry_missing_uses_default)
615+
{
616+
// Test that missing parameters use their default values
617+
618+
conf_file_writer json_config(R"({
619+
"performance": {
620+
"rings": {
621+
"tx": {}
622+
}
623+
}
624+
})");
625+
626+
std::queue<std::unique_ptr<loader>> loaders;
627+
loaders.push(std::make_unique<json_loader>(json_config.get()));
628+
629+
config_registry registry(
630+
std::move(loaders),
631+
std::make_unique<json_descriptor_provider>(sample_type_validation_descriptor));
632+
633+
ASSERT_EQ(16LL, registry.get_value<int64_t>("performance.rings.tx.udp_buffer_batch"));
634+
}

tests/unit_tests/config/json_descriptor_provider.cpp

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -601,10 +601,8 @@ TEST(config, json_descriptor_provider_memory_size_transformer_applied)
601601
int64_t transformed_value = std::experimental::any_cast<int64_t>(transformed);
602602
ASSERT_EQ(1073741824LL, transformed_value); // 1GB in bytes
603603

604-
// Test that parameter without transformer doesn't transform strings
605-
std::experimental::any untransformed = without_transformer.get_value(gb_string);
606-
std::string untransformed_value = std::experimental::any_cast<std::string>(untransformed);
607-
ASSERT_EQ("1GB", untransformed_value); // Should remain as string
604+
// Test that parameter without transformer throws an exception
605+
ASSERT_THROW(without_transformer.get_value(gb_string), std::experimental::bad_any_cast);
608606
}
609607

610608
/**

tests/unit_tests/config/parameter_descriptor.cpp

Lines changed: 63 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -385,4 +385,66 @@ TEST_F(memory_size_transformer, test_overflow)
385385
{
386386
// A very large number without suffix that fits uint64_t but not int64_t
387387
ASSERT_THROW(transformer(std::string("10000000000000000000")), xlio_exception);
388-
}
388+
}
389+
390+
TEST(config, parameter_descriptor_valid_integer_passes)
391+
{
392+
// Test that valid integer values pass through unchanged
393+
constexpr int64_t DEFAULT_INT_VALUE = 16;
394+
parameter_descriptor int_param_descriptor {std::experimental::any(int64_t(DEFAULT_INT_VALUE))};
395+
396+
std::experimental::any valid_int = int64_t(32);
397+
std::experimental::any result = int_param_descriptor.get_value(valid_int);
398+
ASSERT_EQ(typeid(int64_t), result.type());
399+
ASSERT_EQ(32LL, std::experimental::any_cast<int64_t>(result));
400+
}
401+
402+
TEST(config, parameter_descriptor_boolean_rejected)
403+
{
404+
// Test that boolean values are rejected for integer parameters (this was the bug)
405+
constexpr int64_t DEFAULT_INT_VALUE = 16;
406+
parameter_descriptor int_param_descriptor {std::experimental::any(int64_t(DEFAULT_INT_VALUE))};
407+
408+
std::experimental::any bool_value = true;
409+
ASSERT_THROW(int_param_descriptor.get_value(bool_value), std::experimental::bad_any_cast);
410+
}
411+
412+
TEST(config, parameter_descriptor_string_rejected)
413+
{
414+
// Test that string values are rejected for integer parameters
415+
constexpr int64_t DEFAULT_INT_VALUE = 16;
416+
parameter_descriptor int_param_descriptor {std::experimental::any(int64_t(DEFAULT_INT_VALUE))};
417+
418+
std::experimental::any string_value = std::string("invalid");
419+
ASSERT_THROW(int_param_descriptor.get_value(string_value), std::experimental::bad_any_cast);
420+
}
421+
422+
TEST(config, parameter_descriptor_double_rejected)
423+
{
424+
// Test that double values are rejected for integer parameters
425+
constexpr int64_t DEFAULT_INT_VALUE = 16;
426+
parameter_descriptor int_param_descriptor {std::experimental::any(int64_t(DEFAULT_INT_VALUE))};
427+
428+
std::experimental::any double_value = 3.14;
429+
ASSERT_THROW(int_param_descriptor.get_value(double_value), std::experimental::bad_any_cast);
430+
}
431+
432+
TEST(config, parameter_descriptor_boolean_accepts_boolean)
433+
{
434+
// Test that boolean parameters accept boolean values
435+
parameter_descriptor bool_param_descriptor(std::experimental::any(bool(false)));
436+
437+
std::experimental::any bool_value = true;
438+
std::experimental::any result = bool_param_descriptor.get_value(bool_value);
439+
ASSERT_EQ(typeid(bool), result.type());
440+
ASSERT_EQ(true, std::experimental::any_cast<bool>(result));
441+
}
442+
443+
TEST(config, parameter_descriptor_boolean_rejects_integer)
444+
{
445+
// Test that boolean parameters reject integer values
446+
parameter_descriptor bool_param_descriptor(std::experimental::any(bool(false)));
447+
448+
std::experimental::any int_value = int64_t(1);
449+
ASSERT_THROW(bool_param_descriptor.get_value(int_value), std::experimental::bad_any_cast);
450+
}

0 commit comments

Comments
 (0)