Skip to content

Commit 2bb902f

Browse files
committed
fix: resolve 10 correctness, safety, and performance issues
1. Prevent infinite recursion in leetify() on unmappable inputs (e.g. single consonant "M") by capping recursion at max_depth. 2. Filter empty parts in split_name() so consecutive spaces no longer produce empty strings that cause UB in initials(), first_plus_initial(), and initial_plus_last(). 3. Guard mix_two() against single-character name parts where engine.get(2, 1) triggered UB (min > max in distribution). 4. Skip empty lines in parse_file() to prevent empty strings in word lists that would cause UB in transforms calling .back(). 5. Strip trailing \r in parse_file() for cross-platform compatibility with \r\n line endings. 6. Always use name-based generation when a name is provided but no word lists are loaded — previously threw 75% of the time. 7. Fix leetify() force-retry to compare against pre-transform value instead of _original_string, which was almost never equal for name-based nicknames. 8. Switch _engine from mt19937 to mt19937_64 and XOR-fold seeds for per-call engines so upper 32 bits are no longer discarded. 9. Move word vector in parse_file() instead of copying it. 10. Replace character-based candidate list in oneleet() with position-based selection to eliminate duplicate bias.
1 parent a3ef2f9 commit 2bb902f

2 files changed

Lines changed: 188 additions & 51 deletions

File tree

dasmig/nicknamegen.hpp

Lines changed: 92 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -139,8 +139,8 @@ class nng
139139
{
140140
auto call_seed = static_cast<std::uint64_t>(_engine());
141141
effolkronium::random_local call_engine;
142-
call_engine.seed(
143-
static_cast<std::mt19937::result_type>(call_seed));
142+
call_engine.seed(static_cast<std::mt19937::result_type>(
143+
(call_seed ^ (call_seed >> 32U))));
144144
auto result = solver(name, call_engine);
145145
result._seed = call_seed;
146146
return result;
@@ -160,8 +160,8 @@ class nng
160160
std::uint64_t call_seed) const
161161
{
162162
effolkronium::random_local call_engine;
163-
call_engine.seed(
164-
static_cast<std::mt19937::result_type>(call_seed));
163+
call_engine.seed(static_cast<std::mt19937::result_type>(
164+
(call_seed ^ (call_seed >> 32U))));
165165
auto result = solver(name, call_engine);
166166
result._seed = call_seed;
167167
return result;
@@ -179,7 +179,7 @@ class nng
179179
/// @return `*this` for chaining.
180180
nng& seed(std::uint64_t seed_value)
181181
{
182-
_engine.seed(static_cast<std::mt19937::result_type>(seed_value));
182+
_engine.seed(seed_value);
183183
return *this;
184184
}
185185

@@ -253,7 +253,7 @@ class nng
253253
std::vector<word_container> _wordlists;
254254

255255
// Per-instance random engine for seed drawing.
256-
std::mt19937 _engine{std::random_device{}()};
256+
std::mt19937_64 _engine{std::random_device{}()};
257257

258258
// Tag type for the auto-probing singleton constructor.
259259
struct auto_probe_tag {};
@@ -420,27 +420,22 @@ class nng
420420
// Nickname with leetified letter.
421421
std::wstring leet_nickname{nickname};
422422

423-
// Candidate letters to be leetified.
424-
std::vector<wchar_t> candidates;
423+
// Collect indices of leet-mappable characters.
424+
std::vector<std::size_t> candidate_positions;
425425

426-
// Retrieve candidates for replacement.
427-
for (const auto& character : leet_nickname)
426+
for (std::size_t i = 0; i < leet_nickname.size(); ++i)
428427
{
429-
if (_leet_map.contains(character))
428+
if (_leet_map.contains(leet_nickname[i]))
430429
{
431-
candidates.push_back(character);
430+
candidate_positions.push_back(i);
432431
}
433432
}
434433

435-
// Pick one candidate at random and replace its first occurrence.
436-
if (!candidates.empty())
434+
// Pick one position at random and replace it.
435+
if (!candidate_positions.empty())
437436
{
438-
auto target = *engine.get(candidates);
439-
if (auto it = std::ranges::find(leet_nickname, target);
440-
it != leet_nickname.end())
441-
{
442-
*it = _leet_map.at(target);
443-
}
437+
auto pos = *engine.get(candidate_positions);
438+
leet_nickname[pos] = _leet_map.at(leet_nickname[pos]);
444439
}
445440

446441
return leet_nickname;
@@ -471,8 +466,12 @@ class nng
471466
// NOLINTNEXTLINE(misc-no-recursion)
472467
static nickname leetify(nickname nickname,
473468
effolkronium::random_local& engine,
474-
bool force = false)
469+
bool force = false, int depth = 0)
475470
{
471+
// Maximum recursion depth to prevent stack overflow on unmappable
472+
// inputs.
473+
static constexpr int max_depth{7};
474+
476475
// We have 1/2 chance of leetifying, force parameter overrides this.
477476
if (force || engine.get<bool>())
478477
{
@@ -488,32 +487,40 @@ class nng
488487
allleet // n1ckn4m3
489488
};
490489

490+
// Capture pre-transform value to detect no-ops.
491+
auto before = nickname._internal_string;
492+
491493
// New leetified nickname.
492494
nickname._internal_string =
493495
(*engine.get(
494496
possible_generators))(nickname, engine);
495497

496-
// If the new nickname didn't suffer any alteration, force
497-
// leetify again.
498-
return leetify(nickname, engine,
499-
nickname._internal_string ==
500-
nickname._original_string);
498+
// If the new nickname didn't change and we haven't exceeded
499+
// the depth limit, force leetify again.
500+
if (depth < max_depth)
501+
{
502+
return leetify(nickname, engine,
503+
nickname._internal_string == before,
504+
depth + 1);
505+
}
501506
}
507+
else
508+
{
509+
// Possible methods utilized to leetify the nickname.
510+
static const generators possible_generators{
511+
xfy, // nicknameX
512+
reverse, // emanckin
513+
yfy, // nicknamy
514+
numify, // nickname2000
515+
tracefy, // nickname-
516+
ingify, // nicknaming
517+
};
502518

503-
// Possible methods utilized to leetify the nickname.
504-
static const generators possible_generators{
505-
xfy, // nicknameX
506-
reverse, // emanckin
507-
yfy, // nicknamy
508-
numify, // nickname2000
509-
tracefy, // nickname-
510-
ingify, // nicknaming
511-
};
512-
513-
// New leetified nickname.
514-
nickname._internal_string =
515-
(*engine.get(possible_generators))(
516-
nickname, engine);
519+
// New leetified nickname.
520+
nickname._internal_string =
521+
(*engine.get(possible_generators))(
522+
nickname, engine);
523+
}
517524
}
518525

519526
return nickname;
@@ -761,15 +768,20 @@ class nng
761768
}
762769

763770
// Split a full name into a vector containing each name/surname.
771+
// Empty parts (from consecutive spaces) are filtered out.
764772
[[nodiscard]] static std::vector<std::wstring> split_name(
765773
const std::wstring& name)
766774
{
767775
std::vector<std::wstring> parts;
768776

769777
for (auto part : name | std::views::split(L' '))
770778
{
771-
parts.emplace_back(std::ranges::begin(part),
772-
std::ranges::end(part));
779+
std::wstring s(std::ranges::begin(part),
780+
std::ranges::end(part));
781+
if (!s.empty())
782+
{
783+
parts.push_back(std::move(s));
784+
}
773785
}
774786

775787
return parts;
@@ -829,17 +841,25 @@ class nng
829841
auto names_list{split_name(name)};
830842

831843
// Reduce name list to two names.
832-
while (names_list.size() > 2)
844+
if (names_list.size() > 2)
833845
{
834-
names_list.erase(names_list.begin());
846+
names_list.erase(names_list.begin(),
847+
names_list.end() - 2);
835848
}
836849

837850
// Iterate through each name retrieving random number of letters.
838851
for (const auto& name : names_list)
839852
{
840-
nickname.append(name.substr(
841-
0, engine.get<std::size_t>(
842-
2, name.size())));
853+
if (name.size() < 2)
854+
{
855+
nickname.append(name);
856+
}
857+
else
858+
{
859+
nickname.append(name.substr(
860+
0, engine.get<std::size_t>(
861+
2, name.size())));
862+
}
843863
}
844864

845865
return nickname;
@@ -914,9 +934,16 @@ class nng
914934
// 1/4 chance of nickname being name related.
915935
static constexpr double name_related_probability{0.25};
916936

937+
// When name is provided, use name-based generation with
938+
// name_related_probability — but always use the name if there are
939+
// no wordlists to fall back to.
940+
const bool use_name =
941+
!name.empty() &&
942+
(_wordlists.empty() ||
943+
engine.get<bool>(name_related_probability));
944+
917945
// Proceed to generate nickname based on name.
918-
if (!name.empty() && engine.get<bool>(
919-
name_related_probability))
946+
if (use_name)
920947
{
921948
// Possible methods utilized to generate a nickname.
922949
// Purposefully adds redundancy to first and last name with any name
@@ -979,11 +1006,25 @@ class nng
9791006
// Retrieves list of words.
9801007
while (std::getline(tentative_file, file_line, delimiter))
9811008
{
982-
words_read.push_back(file_line);
1009+
// Strip trailing carriage return for cross-platform
1010+
// compatibility.
1011+
if (!file_line.empty() && file_line.back() == L'\r')
1012+
{
1013+
file_line.pop_back();
1014+
}
1015+
1016+
// Skip empty lines (including blank trailing lines).
1017+
if (!file_line.empty())
1018+
{
1019+
words_read.push_back(file_line);
1020+
}
9831021
}
9841022

9851023
// Index our container.
986-
_wordlists.push_back(words_read);
1024+
if (!words_read.empty())
1025+
{
1026+
_wordlists.push_back(std::move(words_read));
1027+
}
9871028
}
9881029
}
9891030

tests/tests.cpp

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,10 @@ struct nng_test_access
150150
{
151151
return g._wordlists;
152152
}
153+
static std::vector<std::wstring> split_name(const std::wstring& s)
154+
{
155+
return dasmig::nng::split_name(s);
156+
}
153157
};
154158

155159
// ---------------------------------------------------------------------------
@@ -929,3 +933,95 @@ TEST_CASE("singleton still works after multi-instance changes",
929933
auto nick = g.get_nickname(L"Singleton Check");
930934
REQUIRE_FALSE(static_cast<std::wstring>(nick).empty());
931935
}
936+
937+
// ===== Robustness / edge-case tests ========================================
938+
939+
TEST_CASE("split_name filters consecutive spaces", "[nng][robustness]")
940+
{
941+
// Double space should not produce empty parts.
942+
auto parts = nng_test_access::split_name(L"John Doe");
943+
REQUIRE(parts.size() == 2);
944+
REQUIRE(parts[0] == L"John");
945+
REQUIRE(parts[1] == L"Doe");
946+
947+
// Leading/trailing spaces.
948+
auto parts2 = nng_test_access::split_name(L" Jane ");
949+
REQUIRE(parts2.size() == 1);
950+
REQUIRE(parts2[0] == L"Jane");
951+
}
952+
953+
TEST_CASE("initials handles consecutive spaces safely", "[nng][robustness]")
954+
{
955+
// Should not crash — previously UB with empty parts.
956+
auto result = nng_test_access::initials(L"John Doe");
957+
REQUIRE(result.size() == 2);
958+
}
959+
960+
TEST_CASE("mix_two handles single-character name parts", "[nng][robustness]")
961+
{
962+
// "Li A" has a 1-char part — previously UB (min > max in distribution).
963+
for (int i = 0; i < 50; ++i)
964+
{
965+
auto nick = nng_test_access::mix_two(L"Li A");
966+
REQUIRE_FALSE(nick.empty());
967+
}
968+
}
969+
970+
TEST_CASE("leetify does not stack-overflow on unmappable input",
971+
"[nng][robustness]")
972+
{
973+
// "M" (single consonant, no leet map entry) previously caused infinite
974+
// recursion. Now capped at max depth.
975+
dasmig::nng g;
976+
g.load("resources");
977+
g.seed(1);
978+
for (int i = 0; i < 20; ++i)
979+
{
980+
auto nick = g.get_nickname(L"M");
981+
REQUIRE_FALSE(static_cast<std::wstring>(nick).empty());
982+
}
983+
}
984+
985+
TEST_CASE("name-only generation works without wordlists", "[nng][robustness]")
986+
{
987+
dasmig::nng g; // No load() — wordlists empty.
988+
989+
// Every call should succeed (previously 75% threw).
990+
for (int i = 0; i < 50; ++i)
991+
{
992+
auto nick = g.get_nickname(L"John Smith");
993+
REQUIRE_FALSE(static_cast<std::wstring>(nick).empty());
994+
}
995+
}
996+
997+
TEST_CASE("no-name no-wordlists still throws", "[nng][robustness]")
998+
{
999+
dasmig::nng g;
1000+
REQUIRE_THROWS_AS(g.get_nickname(), std::invalid_argument);
1001+
}
1002+
1003+
TEST_CASE("64-bit seeds differing in upper bits produce different nicknames",
1004+
"[nng][robustness]")
1005+
{
1006+
auto& g = gen();
1007+
1008+
// Two seeds identical in lower 32 bits but different in upper 32 bits.
1009+
constexpr std::uint64_t seed_lo = 42;
1010+
constexpr std::uint64_t seed_hi =
1011+
42 | (static_cast<std::uint64_t>(1) << 32U);
1012+
1013+
bool found_different = false;
1014+
for (int i = 0; i < 100; ++i)
1015+
{
1016+
auto a = static_cast<std::wstring>(
1017+
g.get_nickname(L"Bit Test", seed_lo + i));
1018+
auto b = static_cast<std::wstring>(
1019+
g.get_nickname(L"Bit Test", seed_hi + i));
1020+
if (a != b)
1021+
{
1022+
found_different = true;
1023+
break;
1024+
}
1025+
}
1026+
REQUIRE(found_different);
1027+
}

0 commit comments

Comments
 (0)