Conversation
ついでにAESのコンストラクタを完全に定数計算対応に
変更・追加 GetBuffer -> GetRawPacketにリネーム GetRawData -> イミュータブルな生データ参照を返す RefRawData -> ミュータブルな生データ参照を返す 削除 SetBuffer -> ヘッダーを考慮しないバッファが渡される可能性、代替として前述のRefRawDataを実装したので削除
以下のようなエイリアスを追加 `Packet::header_bytes = std::array<byte_t, HeaderSize>`
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthroughソケット/パケットAPIを再設計し、名前空間を NetIO→Socket に変更。Send/Recv を view/ref ベースへ移行し、Packet にヘッダ/データ分離を追加。MultiWordInt に高速乗算や比較拡張を導入。複数のネットワーク／暗号／シリアライズ例を新規追加。 Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ClientSocket as "Client Socket"
participant ServerSocket as "Server Socket"
participant Server
participant CryptEngine as "CryptEngine"
Client->>ClientSocket: connect(addr:port)
Client->>ClientSocket: send Packet (GetRawPacket / byte_view)
ClientSocket->>CryptEngine: Encrypt(byte_view)
ClientSocket->>ServerSocket: transmit bytes
ServerSocket->>CryptEngine: Decrypt -> Packet reconstructed (GetRawData)
CryptEngine-->>Server: deliver Packet
Server->>Server: process, prefix with client name
Server->>ServerSocket: broadcast Packet to other clients
ServerSocket->>CryptEngine: Encrypt(byte_view)
ServerSocket->>ClientSocket: deliver bytes
ClientSocket->>CryptEngine: Decrypt -> Client displays message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
include/Cryptgraphy/MultiWordInt.h (1)
519-519:⚠️ Potential issue | 🔴 Critical文字列変換テーブルに文字順の誤りがあります:
"kmnlop"→"klmnop"複数箇所のルックアップテーブルで
'k'の後が"mnlop"になっていますが、正しくは"lmnop"です。影響範囲:
- Line 519, 535, 536:
CharSpanToWord/WordToString— base > 20 での解析・出力が不正- Line 565:
Parseのfind_first_not_of— 同上- Line 631:
ToBase64— Base64エンコード結果が不正（'K','N','M','L'と'k','n','m','l'の位置が入れ替わっている）- Line 666:
ToString— 同上base 16（hex）では影響しませんが、base > 20 および Base64 で誤った結果を返します。
🐛 修正例（一部抜粋）
Line 519:
- constexpr std::string_view list = "0123456789abcdefghijkmnlopqrstuvwxyz"; + constexpr std::string_view list = "0123456789abcdefghijklmnopqrstuvwxyz";Line 535-536:
- constexpr std::string_view listlower = "0123456789abcdefghijkmnlopqrstuvwxyz"; - constexpr std::string_view listupper = "0123456789ABCDEFGHIJKMNLOPQRSTUVWXYZ"; + constexpr std::string_view listlower = "0123456789abcdefghijklmnopqrstuvwxyz"; + constexpr std::string_view listupper = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ";Line 631:
- constexpr std::string_view list = "ABCDEFGHIJKNMLOPQRSTUVWXYZabcdefghijknmlopqrstuvwxyz0123456789+/"; + constexpr std::string_view list = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/";Line 666:
- constexpr std::string_view list = "0123456789abcdefghijkmnlopqrstuvwxyz"; + constexpr std::string_view list = "0123456789abcdefghijklmnopqrstuvwxyz";Also applies to: 535-536, 565-565, 631-631, 666-666
include/Cryptgraphy/SHAKE256.h (1)
91-98:⚠️ Potential issue | 🟠 Major
operator bytearray()内でも同じstd::bit_castポインタ問題があります。Line 95 の
std::bit_cast<const byte_t*>(m_words.data()) + iも上記コンストラクタと同じ問題を持ちます。constexprコンテキストでの使用を想定する場合、バイトレベルのインデックスアクセスに変更が必要です。constexpr 安全な代替案
constexpr operator bytearray() const { bytearray ret; ret.reserve(sizeof(m_words)); for (size_t i = 0, c = sizeof(m_words); i < c; ++i) { - ret.push_back(*(std::bit_cast<const byte_t*>(m_words.data()) + i)); + size_t word_idx = i / sizeof(word_t); + size_t byte_idx = i % sizeof(word_t); + ret.push_back(static_cast<byte_t>(m_words[word_idx] >> (byte_idx * 8))); } return ret; }include/Packet.h (1)
276-289:⚠️ Potential issue | 🔴 CriticalLine 282 の
byte_view初期化が誤っている
byte_view(m_buffer.begin(), HeaderSize)はヘッダ部分の最初のHeaderSizeバイト分のビューを作成します。GetArray は データ部分（ヘッダの後）を処理すべきです。他の類似メソッド（line 217, line 250）では.subspan(HeaderSize)を使用してヘッダをスキップしているため、同じパターンに統一してください。- byte_view view = byte_view(m_buffer.begin(), HeaderSize); + byte_view view = byte_view(m_buffer).subspan(HeaderSize);
🤖 Fix all issues with AI agents
In `@example/KeyExchange/KeyExchange.cpp`:
- Around line 23-30: The byte-print loops over Ga and Gb can sign-extend when b
is a signed char; change the casts in both loops (the iterations over Ga and Gb
in KeyExchange.cpp) to static_cast<int>(static_cast<unsigned char>(b)) so values
>= 0x80 are printed as two hex digits instead of expanded negative ints; update
both occurrences where (int)b is used.
In `@example/Network/Network.cpp`:
- Around line 39-44: The code calls std::stoi(args[1]) without validation which
can throw std::invalid_argument or std::out_of_range; wrap the conversion in a
try-catch (catch both exceptions) or pre-validate args[1] before conversion, and
handle errors by logging and exiting or using a safe default instead of calling
Server() or Client(); update the block around the std::stoi call that decides
between Server() and Client() to perform the safe conversion and error handling
for args[1].
- Around line 104-109: The loop dereferences p->GetPeerAddress() without
checking the std::optional, causing UB when GetPeerAddress() returns nullopt;
update the loop that processes lostqueue to first test the optional (e.g., if
(auto addr = p->GetPeerAddress()) or addr.has_value()) and only call
clients.erase(*addr) when present, otherwise handle the nullopt case (skip
erase, log a warning, or use an alternative key) so no dereference of nullopt
occurs; modify the block around lostqueue, p, GetPeerAddress(), and
clients.erase accordingly.
- Line 181: The global/local variable stopflag is accessed from both the input
thread and the main thread causing a data race; change its type to
std::atomic<bool> (e.g., std::atomic<bool> stopflag{false};), add `#include`
<atomic>, and replace raw reads/writes in the input thread and main thread with
atomic operations (use stopflag.store(true) for writes and stopflag.load() for
reads or the atomic's operator bool/operator=) to ensure thread-safe
synchronization in the code that references stopflag.
- Around line 94-101: EncryptionRecv() returns std::optional<Packet> and is
being dereferenced without checking, causing UB if it returns nullopt; change
the chain to first capture the optional (e.g., auto pkt = c->EncryptionRecv())
and check it before calling pkt->Get<ClientData>(), then proceed to log and move
client into clients using c->GetPeerAddress() and clients[*addr] =
{std::move(*c), std::move(*cd)}; ensure you only call Get<ClientData>() and use
cd when pkt has value and handle the else/no-packet case appropriately.
- Line 126: The code dereferences optional results from val->Get<std::string>()
and pak->Get<std::string>() without checking for a value, which is undefined
behavior if empty; update the places building the message (the line constructing
the local variable send that uses cd.Name and cd.Level and the later use at
pak->Get<std::string>()) to first test the returned std::optional (e.g., with
has_value() or by using value_or) and handle the empty case (skip/early-return,
use a default string, or log an error) so you never directly dereference the
optional; make the check/handling adjacent to the constructs that call
Get<std::string>() so behavior is consistent.
In `@include/Cryptgraphy/MultiWordInt.h`:
- Around line 375-418: The Karatuba implementation shares temporaries (t and
z0/z1/z2) across recursion levels causing overwrites; to fix, make the recursive
helper return bigint by value and declare per-call temporaries inside the helper
instead of as outer-scope locals: move t (t[4]) and z0,z1,z2 into the lambda
body (the rec lambda in Karatuba) so each recursion has its own instances,
update the lambda signature from -> bigint& to -> bigint and adjust all uses
(calls to self(self, ...) and the final return) accordingly, ensuring
FromWords/MulBase/AssignAdd/AssignLeftShift operate on the local temporaries.
- Around line 891-907: AssignRightShift currently reads m_words[i + 1] out of
bounds and miscomputes bitshift when n is a multiple of WordBits; fix by
computing count_t whole = n / WordBits and count_t rem = n % WordBits (replace
wordshift/bitshift calc), then if rem==0 set bitshift=0 and avoid calling
WordShiftBase (use direct copy of m_words[i] into m_words[i - whole]) and only
iterate i from whole to c-1 ensuring you never access i+1 when rem==0; when
rem!=0 iterate i from whole to c-2 (or use condition i+1 < c) and call
WordShiftBase(m_words[i], m_words[i+1], WordBits - rem) so m_words[i+1] is
always in-range; finally zero out the high 'whole' words (use std::fill on the
rbegin slice) and ensure the tail element written from the last source is set to
0 if needed. Reference symbols: AssignRightShift, m_words, GetWords,
WordShiftBase, WordBits, wordshift/bitshift.
- Around line 874-890: AssignLeftShift can underflow when wordshift > GetWords()
and misuses WordShiftBase when bitshift == 0; fix by first checking if wordshift
>= GetWords() and in that case set all m_words to 0 and return *this, otherwise
compute count_t srcWords = GetWords() - wordshift safely and iterate only over
srcWords; also avoid calling WordShiftBase when bitshift == 0 by using a branch:
m_words[i + wordshift] = (bitshift == 0 ? m_words[i] :
WordShiftBase(i==0?0:m_words[i-1], m_words[i], bitshift)); this uses the
existing AssignLeftShift, GetWords, m_words, WordShiftBase, wordshift and
bitshift symbols.
- Around line 800-804: GetNBit currently subtracts 1 from GetWords() causing
underflow and out-of-bounds access when m_words is empty; update GetNBit (and
use the same safe loop pattern as GetNWord) to handle zero words: if GetWords()
== 0 return 0 (or GetAllBits() if that semantics applies) otherwise scan words
from the last down to find the highest nonzero word and compute bits using
std::bit_width(m_words[idx]) with WordBits and GetAllBits fallback; reference
functions/fields: GetNBit, GetWords, GetNWord, m_words, WordBits, GetAllBits.
- Around line 922-950: Add explicit equality operators for the variable-length
bigint: implement friend constexpr bool operator==(const bigint& lhs, const
bigint& rhs) noexcept and friend constexpr bool operator!=(const bigint& lhs,
const bigint& rhs) noexcept and have them use the existing Compare(const bigint&
lhs, const bigint& rhs) noexcept (or the member Compare(rhs) const) to return
true when Compare(...) yields std::strong_ordering::equal and false otherwise;
place these alongside the existing friend operator<=> so equality is properly
defined for the variable-length bigint.
- Around line 816-826: AssignAdd currently loops to this->GetWords() and
unconditionally accesses rhs.m_words[i], causing OOB if rhs has fewer words;
change AssignAdd to first get count_t rhsWords = rhs.GetWords() then loop i from
0 to min(this->GetWords(), rhsWords) and call AddBase(rhs.m_words[i],
std::addressof(this->m_words[i]), carry); after that, if carry remains and
this->GetWords() > rhsWords, continue a second loop from i = rhsWords to
this->GetWords() calling AddBase(0, std::addressof(this->m_words[i]), carry)
until carry clears (or you reach the end); use the bigint::AssignAdd, GetWords,
m_words and AddBase identifiers to locate the code and avoid any direct
rhs.m_words[i] access beyond rhs.GetWords().
- Around line 909-911: The unary bitwise NOT operator operator~() currently
mutates *this and returns a reference via AssignNOT(), which is surprising;
change operator~() to be non-destructive and return a new bigint by value
(mirroring the fixed-length variant). Implement it by making a copy of *this and
calling AssignNOT() on the copy (e.g., bigint tmp(*this); tmp.AssignNOT();
return tmp), adjust the signature to return bigint (and mark const/noexcept
appropriately), and remove the in-place behavior from operator~() that returns
bigint&.
- Around line 871-873: WordShiftBase performs a shift using (WordBits - n) which
becomes WordBits when n==0, causing undefined behavior; update WordShiftBase to
handle n==0 explicitly (e.g., return low when n==0) before performing (low >> n)
| (high << (WordBits - n)); reference the function WordShiftBase(word_t low,
word_t high, count_t n) noexcept and the constant WordBits (and note that
AssignLeftShift computes bitshift = n & (WordBits - 1) so n can be a multiple of
WordBits), and add the minimal conditional branch to avoid shifting by WordBits.
- Around line 828-836: The AssignNOT() constexpr method uses the
execution-policy overload of std::transform which is not constexpr; replace the
call that passes std::execution::unseq with the policy-less overload of
std::transform so the function can be evaluated in a constant expression. Locate
AssignNOT(), use std::transform(m_words.begin(), m_words.end(), m_words.begin(),
std::bit_not<word_t>()) (keeping the same m_words container and
std::bit_not<word_t>() functor) and keep the constexpr and noexcept specifiers
and return *this as before.
In `@include/Cryptgraphy/SHAKE256.h`:
- Around line 21-30: The constexpr state(byte_view from) currently uses
std::bit_cast<byte_t*> on m_words which is forbidden in constexpr; replace the
pointer-based copy with an index-based byte writer: iterate i from 0 to b (or
until from ends) and for each byte compute the target word index and byte offset
within m_words, then update m_words[word_index] by masking out the target byte
and OR-ing in (static_cast<word_t>(from[i]) << (offset*8)) so all writes are
done via integer operations (no pointer casts). Keep references to the same
symbols: state(byte_view), m_words, byte_t, byte_view, b and ensure the loop and
masking/shifting are constexpr-friendly.
In `@include/Packet.h`:
- Line 189: The constructor template Packet(enumT type, const std::vector<T>&
data) has the requires clause checking is_enum32<T> but it should check the enum
template parameter; update the constraint to use is_enum32<enumT> (i.e.,
requires (is_enum32<enumT>&& cross_convertible<T>)) so the enum parameter enumT
is validated correctly, matching the other overloads like the one at Line 167.
In `@include/Socket.h`:
- Around line 555-559: The lambda in ASyncSend is capturing byte_view parameter
src by reference which can dangle once ASyncSend returns; change the async task
to capture src by value so the span remains valid for the asynchronous call
(i.e., invoke std::async with a lambda that captures src by value and calls
this->Send(src)); apply the same fix to ASyncEncryptionSend(byte_view src) so
both use value-capture of src instead of [&].
In `@Socket.cpp`:
- Around line 28-47: The main function currently calls std::stoi(args[1])
without validation which can throw std::invalid_argument or std::out_of_range;
wrap the conversion in a try-catch or pre-validate the string (e.g., check
digits and optional sign or use std::from_chars) and handle errors by printing a
usage/error message and returning non-zero; specifically update main to guard
the std::stoi call (or replace it) and only call Server() or Client() when the
parsed value is valid, otherwise return an error code and avoid uncaught
exceptions.
🟡 Minor comments (3)
include/Cryptgraphy/MultiWordInt.h-294-328 (1)
294-328:⚠️ Potential issue | 🟡 Minor
NormalMulの実装 — おおむね正しいですが、Line 317 のキャリー伝播に注意Line 317:
carry = upper + carryflag—upperがUINT64_MAXでcarryflagがtrueの場合、carryがラップして0になり、上位ワードへの繰り上げが失われます。発生確率は低いですが、大きな値の乗算で不正な結果になる可能性があります。example/EC_Signature/EC_Signature.cpp-22-24 (1)
22-24:⚠️ Potential issue | 🟡 Minor
ret出力後の改行が欠落しています。Line 22 で
std::boolalpha << retを出力した後に改行がありません。また、assert(ret)はリリースビルド（NDEBUG定義時）では無効化されるため、検証失敗を確実に検出したい場合は明示的なチェックを検討してください。提案
- std::cout << std::boolalpha << ret; - - assert(ret); + std::cout << std::boolalpha << ret << std::endl; + + assert(ret);include/Cryptgraphy/AES128.h-237-246 (1)
237-246:⚠️ Potential issue | 🟡 Minor
std::bit_cast<byte_t*>(m_bytes.data())は冗長なキャストです。
m_bytesはstd::array<byte_t, block_size>なので、m_bytes.data()は既にbyte_t*を返します。std::bit_cast<byte_t*>で同じ型へキャストするのは不要であり、constexprコンテキストにおけるポインタのstd::bit_castはコンパイラによって定数式として拒否される可能性があります。SHAKE256.h のパターンと統一しようとしたと思われますが、ここでは不要です。直接ポインタを使用できます。
提案
constexpr block_t(byte_view from) noexcept { - auto it = std::bit_cast<byte_t*>(m_bytes.data()); - auto end = std::bit_cast<byte_t*>(m_bytes.data() + block_size); + auto it = m_bytes.data(); + auto end = m_bytes.data() + block_size; for (auto&& c : from) { *it = c; if (++it == end) { break; } } }
🧹 Nitpick comments (11)
example/KeyExchange/KeyExchange.cpp (2)
16-20:std::chrono::duration<double>を使った方がより簡潔手動の
/ 1000 / 1000 / 1000変換はstd::chrono::duration<double>で置き換えるとより読みやすくなります。♻️ リファクタ案
- auto ns = std::chrono::duration_cast<std::chrono::nanoseconds>(std::chrono::high_resolution_clock::now() - tp).count(); - - bool same = Ga == Gb; - - std::cout << (double)ns / 1000 / 1000 / 1000 << "s" << std::endl; + auto elapsed = std::chrono::duration<double>(std::chrono::high_resolution_clock::now() - tp); + + bool same = Ga == Gb; + + std::cout << elapsed.count() << "s" << std::endl;
3-3: 未使用のargc/argvパラメータ
argcとargvは使用されていません。シンプルにint main()で十分です。include/Cryptgraphy/MultiWordInt.h (2)
810-815: 固定長版と可変長版でAddBaseのパラメータ順序が異なります固定長版 (Line 239):
AddBase(word_t *dest, word_t src, bool carry)
可変長版 (Line 810):AddBase(word_t src, word_t *dest, bool carry)同じ名前で引数順が逆転しているため、メンテナンス時に混乱やバグの原因になります。統一を推奨します。
75-75:std::bit_cast<T*>(words().data())はポインタ型間のbit_castであり、UB の懸念があります
std::bit_castはオブジェクト表現のビット単位コピーを行いますが、ポインタ間のbit_castは strict aliasing 違反となる可能性があります。TODO コメントにも記載されていますが、std::memcpyやバイト単位のコピーへの置き換えを検討してください。example/EC_Signature/EC_Signature.cpp (1)
3-6: 未使用パラメータargc,argv。
argcとargvが使用されていません。例示コードなので問題は小さいですが、コンパイラ警告を避けるためにint main()とするか、(void)argc; (void)argv;を追加することを検討してください。include/Cryptgraphy/AES128.h (1)
398-418:ParallelProcessorのエッジケース:sizeが 0 の場合の考慮。
sizeが 0 の場合、palcは(0 / available) + 1 = 1となり、Line 407 のend >= size(0) が即座に true となります。i = tasks.capacity() - 1により最初のイテレーションでstart=0, end=0のタスクが1つ起動され、空のループが走ります。致命的な問題ではありませんが、size == 0の場合は早期リターンした方がクリーンです。CMakeLists.txt (1)
19-21:target_include_directoriesの使用を検討してください。
include_directoriesはグローバルスコープに影響します。モダン CMake のプラクティスとして、ターゲットスコープのtarget_include_directoriesを推奨します。提案
-include_directories( - ${PROJECT_SOURCE_DIR}/include -) +target_include_directories(Socket PRIVATE + ${PROJECT_SOURCE_DIR}/include +)Socket.cpp (1)
7-7: 暗号化キーがソースコードにハードコードされている共有鍵
sharedkeyがリテラルとしてソースに埋め込まれています。さらに、example/Network/Network.cppにも同一の値が重複定義されています。デモ/テスト用途であることは理解していますが、実運用を想定する場合はキーを外部化し、コード中にハードコードしないようにしてください。example/Network/Network.cpp (1)
1-227:Socket.cppとの大規模なコード重複このファイルは
Socket.cppとほぼ同一の内容（main(),Server(),Client(),ClientData,sharedkey）です。共通のロジックを共有ヘッダまたはライブラリに抽出し、各エントリポイントから利用する構成にすることを推奨します。include/Packet.h (1)
125-125: デフォルトコンストラクタ: 余分なセミコロン
Packet() {};のブレース後のセミコロンは冗長です（コンパイルに影響はありません）。♻️ 修正案
- Packet() {}; + Packet() {}include/Socket.h (1)
560-564:ASyncRecvも[&]を使用しているが、明示的なキャプチャが望ましい
[&]は必要以上の変数をキャプチャする可能性があります。[this, &dest]のように明示的にキャプチャすることで意図が明確になります。♻️ 修正案
std::future<bool> ASyncRecv(byte_ref dest) { - return std::async(std::launch::async, [&]() { + return std::async(std::launch::async, [this, &dest]() { return this->Recv(dest); }); }Line 582 も同様:
std::future<bool> ASyncEncryptionRecv(byte_ref dest) { - return std::async(std::launch::async, [&]() { + return std::async(std::launch::async, [this, &dest]() { return this->EncryptionRecv(dest); }); }
| for (auto&& b : Ga) { | ||
| std::cout << std::hex << std::setw(2) << std::setfill('0') << std::right << (int)b; | ||
| } | ||
| std::cout << std::endl; | ||
|
|
||
| for (auto&& b : Gb) { | ||
| std::cout << std::hex << std::setw(2) << std::setfill('0') << std::right << (int)b; | ||
| } |
There was a problem hiding this comment.
(int)b のキャストで符号拡張バグの可能性
b が signed char の場合、0x80以上の値で符号拡張が発生し、setw(2) にもかかわらず ffffff80 のような出力になります。static_cast<int>(static_cast<unsigned char>(b)) を使用してください。
🐛 修正案
for (auto&& b : Ga) {
- std::cout << std::hex << std::setw(2) << std::setfill('0') << std::right << (int)b;
+ std::cout << std::hex << std::setw(2) << std::setfill('0') << std::right << static_cast<int>(static_cast<unsigned char>(b));
}
std::cout << std::endl;
for (auto&& b : Gb) {
- std::cout << std::hex << std::setw(2) << std::setfill('0') << std::right << (int)b;
+ std::cout << std::hex << std::setw(2) << std::setfill('0') << std::right << static_cast<int>(static_cast<unsigned char>(b));
}🤖 Prompt for AI Agents
In `@example/KeyExchange/KeyExchange.cpp` around lines 23 - 30, The byte-print
loops over Ga and Gb can sign-extend when b is a signed char; change the casts
in both loops (the iterations over Ga and Gb in KeyExchange.cpp) to
static_cast<int>(static_cast<unsigned char>(b)) so values >= 0x80 are printed as
two hex digits instead of expanded negative ints; update both occurrences where
(int)b is used.
| if (std::stoi(args[1]) == 0) { | ||
| Server(); | ||
| } | ||
| else { | ||
| Client(); | ||
| } |
There was a problem hiding this comment.
std::stoi が例外をスローする可能性がある
Socket.cpp と同様の問題です。無効な入力に対して std::invalid_argument または std::out_of_range がスローされます。
🤖 Prompt for AI Agents
In `@example/Network/Network.cpp` around lines 39 - 44, The code calls
std::stoi(args[1]) without validation which can throw std::invalid_argument or
std::out_of_range; wrap the conversion in a try-catch (catch both exceptions) or
pre-validate args[1] before conversion, and handle errors by logging and exiting
or using a safe default instead of calling Server() or Client(); update the
block around the std::stoi call that decides between Server() and Client() to
perform the safe conversion and error handling for args[1].
| auto cd = c->EncryptionRecv()->Get<ClientData>(); | ||
|
|
||
| if (cd) { | ||
| std::cout << "connected: " << cd->Name << std::endl; | ||
| auto addr = c->GetPeerAddress(); | ||
| clients[*addr] = {std::move(*c), std::move(*cd)}; | ||
| c.reset(); | ||
| } |
There was a problem hiding this comment.
EncryptionRecv() の戻り値が未チェックのまま連鎖呼び出しされている — null参照でUB
EncryptionRecv() は std::optional<Packet> を返します。std::nullopt が返された場合、->Get<ClientData>() は未定義動作を引き起こします。
🐛 修正案
- auto cd = c->EncryptionRecv()->Get<ClientData>();
+ auto recvResult = c->EncryptionRecv();
+ if (!recvResult) {
+ continue;
+ }
+ auto cd = recvResult->Get<ClientData>();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto cd = c->EncryptionRecv()->Get<ClientData>(); | |
| if (cd) { | |
| std::cout << "connected: " << cd->Name << std::endl; | |
| auto addr = c->GetPeerAddress(); | |
| clients[*addr] = {std::move(*c), std::move(*cd)}; | |
| c.reset(); | |
| } | |
| auto recvResult = c->EncryptionRecv(); | |
| if (!recvResult) { | |
| continue; | |
| } | |
| auto cd = recvResult->Get<ClientData>(); | |
| if (cd) { | |
| std::cout << "connected: " << cd->Name << std::endl; | |
| auto addr = c->GetPeerAddress(); | |
| clients[*addr] = {std::move(*c), std::move(*cd)}; | |
| c.reset(); | |
| } |
🤖 Prompt for AI Agents
In `@example/Network/Network.cpp` around lines 94 - 101, EncryptionRecv() returns
std::optional<Packet> and is being dereferenced without checking, causing UB if
it returns nullopt; change the chain to first capture the optional (e.g., auto
pkt = c->EncryptionRecv()) and check it before calling pkt->Get<ClientData>(),
then proceed to log and move client into clients using c->GetPeerAddress() and
clients[*addr] = {std::move(*c), std::move(*cd)}; ensure you only call
Get<ClientData>() and use cd when pkt has value and handle the else/no-packet
case appropriately.
| while (!lostqueue.empty()) { | ||
| auto p = lostqueue.front(); | ||
| lostqueue.pop_front(); | ||
|
|
||
| clients.erase(*p->GetPeerAddress()); | ||
| } |
There was a problem hiding this comment.
GetPeerAddress() の戻り値が未チェック — nullopt参照でUB
p->GetPeerAddress() は std::optional を返しますが、* で直接参照外しされています。接続が切れたソケットでは getpeername が失敗し std::nullopt を返す可能性が高く、その場合クラッシュします。
🐛 修正案
while (!lostqueue.empty()) {
auto p = lostqueue.front();
lostqueue.pop_front();
- clients.erase(*p->GetPeerAddress());
+ auto addr = p->GetPeerAddress();
+ if (addr) {
+ clients.erase(*addr);
+ }
}🤖 Prompt for AI Agents
In `@example/Network/Network.cpp` around lines 104 - 109, The loop dereferences
p->GetPeerAddress() without checking the std::optional, causing UB when
GetPeerAddress() returns nullopt; update the loop that processes lostqueue to
first test the optional (e.g., if (auto addr = p->GetPeerAddress()) or
addr.has_value()) and only call clients.erase(*addr) when present, otherwise
handle the nullopt case (skip erase, log a warning, or use an alternative key)
so no dereference of nullopt occurs; modify the block around lostqueue, p,
GetPeerAddress(), and clients.erase accordingly.
| continue; | ||
| } | ||
|
|
||
| std::string send = cd.Name + "(" + std::to_string(cd.Level) + "): " + *val->Get<std::string>(); |
There was a problem hiding this comment.
Get<std::string>() の戻り値を未チェックで参照外ししている
val->Get<std::string>() は std::optional<std::string> を返します。* で直接参照外しすると、値が空の場合にUBです。Line 221 (*pak->Get<std::string>()) も同様の問題があります。
🐛 修正案（Line 126）
- std::string send = cd.Name + "(" + std::to_string(cd.Level) + "): " + *val->Get<std::string>();
+ auto msg = val->Get<std::string>();
+ if (!msg) {
+ continue;
+ }
+ std::string send = cd.Name + "(" + std::to_string(cd.Level) + "): " + *msg;🐛 修正案（Line 221）
- auto val = *pak->Get<std::string>();
- std::cout << val << std::endl;
+ auto val = pak->Get<std::string>();
+ if (!val) {
+ continue;
+ }
+ std::cout << *val << std::endl;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| std::string send = cd.Name + "(" + std::to_string(cd.Level) + "): " + *val->Get<std::string>(); | |
| auto msg = val->Get<std::string>(); | |
| if (!msg) { | |
| continue; | |
| } | |
| std::string send = cd.Name + "(" + std::to_string(cd.Level) + "): " + *msg; |
🤖 Prompt for AI Agents
In `@example/Network/Network.cpp` at line 126, The code dereferences optional
results from val->Get<std::string>() and pak->Get<std::string>() without
checking for a value, which is undefined behavior if empty; update the places
building the message (the line constructing the local variable send that uses
cd.Name and cd.Level and the later use at pak->Get<std::string>()) to first test
the returned std::optional (e.g., with has_value() or by using value_or) and
handle the empty case (skip/early-return, use a default string, or log an error)
so you never directly dereference the optional; make the check/handling adjacent
to the constructs that call Get<std::string>() so behavior is consistent.
| static constexpr auto Compare(const bigint& lhs, const bigint& rhs) noexcept { | ||
| count_t words[2] = {lhs.GetWords(), rhs.GetWords()}; | ||
| auto [words_min, words_max] = std::minmax(words[0], words[1]); | ||
|
|
||
| bool is_bigger_l = words_max == words[0]; | ||
|
|
||
| const bigint& longer = is_bigger_l ? lhs : rhs; | ||
| const bigint& shorter = is_bigger_l ? rhs : lhs; | ||
|
|
||
| if (!IsZeroInRef(arr_view(longer.m_words).last(words_max - words_min))) { | ||
| return is_bigger_l ? | ||
| std::strong_ordering::greater : std::strong_ordering::less; | ||
| } | ||
|
|
||
| for (count_t i = words_min; i-- > 0;) { | ||
| auto com = lhs.m_words[i] <=> rhs.m_words[i]; | ||
| if (!std::is_eq(com)) { | ||
| return com; | ||
| } | ||
| } | ||
|
|
||
| return std::strong_ordering::equal; | ||
| } | ||
| constexpr auto Compare(const bigint& rhs) const noexcept { | ||
| return Compare(*this, rhs); | ||
| } | ||
| friend constexpr auto operator<=>(const bigint& lhs, const bigint& rhs) noexcept { | ||
| return Compare(lhs, rhs); | ||
| } |
There was a problem hiding this comment.
可変長版 Compare と operator<=> — operator== が未定義です
C++20 では operator<=> を定義しても operator== は自動生成されません（クラス内で = default の <=> を宣言した場合のみ自動生成）。ユーザー定義の <=> の場合、== と != は別途定義する必要があります。固定長版は明示的に operator== / operator!= を定義していますが（Line 462-463）、可変長版にはありません。
🐛 修正案
friend constexpr auto operator<=>(const bigint& lhs, const bigint& rhs) noexcept {
return Compare(lhs, rhs);
}
+friend constexpr bool operator==(const bigint& lhs, const bigint& rhs) noexcept {
+ return Compare(lhs, rhs) == 0;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static constexpr auto Compare(const bigint& lhs, const bigint& rhs) noexcept { | |
| count_t words[2] = {lhs.GetWords(), rhs.GetWords()}; | |
| auto [words_min, words_max] = std::minmax(words[0], words[1]); | |
| bool is_bigger_l = words_max == words[0]; | |
| const bigint& longer = is_bigger_l ? lhs : rhs; | |
| const bigint& shorter = is_bigger_l ? rhs : lhs; | |
| if (!IsZeroInRef(arr_view(longer.m_words).last(words_max - words_min))) { | |
| return is_bigger_l ? | |
| std::strong_ordering::greater : std::strong_ordering::less; | |
| } | |
| for (count_t i = words_min; i-- > 0;) { | |
| auto com = lhs.m_words[i] <=> rhs.m_words[i]; | |
| if (!std::is_eq(com)) { | |
| return com; | |
| } | |
| } | |
| return std::strong_ordering::equal; | |
| } | |
| constexpr auto Compare(const bigint& rhs) const noexcept { | |
| return Compare(*this, rhs); | |
| } | |
| friend constexpr auto operator<=>(const bigint& lhs, const bigint& rhs) noexcept { | |
| return Compare(lhs, rhs); | |
| } | |
| static constexpr auto Compare(const bigint& lhs, const bigint& rhs) noexcept { | |
| count_t words[2] = {lhs.GetWords(), rhs.GetWords()}; | |
| auto [words_min, words_max] = std::minmax(words[0], words[1]); | |
| bool is_bigger_l = words_max == words[0]; | |
| const bigint& longer = is_bigger_l ? lhs : rhs; | |
| const bigint& shorter = is_bigger_l ? rhs : lhs; | |
| if (!IsZeroInRef(arr_view(longer.m_words).last(words_max - words_min))) { | |
| return is_bigger_l ? | |
| std::strong_ordering::greater : std::strong_ordering::less; | |
| } | |
| for (count_t i = words_min; i-- > 0;) { | |
| auto com = lhs.m_words[i] <=> rhs.m_words[i]; | |
| if (!std::is_eq(com)) { | |
| return com; | |
| } | |
| } | |
| return std::strong_ordering::equal; | |
| } | |
| constexpr auto Compare(const bigint& rhs) const noexcept { | |
| return Compare(*this, rhs); | |
| } | |
| friend constexpr auto operator<=>(const bigint& lhs, const bigint& rhs) noexcept { | |
| return Compare(lhs, rhs); | |
| } | |
| friend constexpr bool operator==(const bigint& lhs, const bigint& rhs) noexcept { | |
| return Compare(lhs, rhs) == 0; | |
| } |
🤖 Prompt for AI Agents
In `@include/Cryptgraphy/MultiWordInt.h` around lines 922 - 950, Add explicit
equality operators for the variable-length bigint: implement friend constexpr
bool operator==(const bigint& lhs, const bigint& rhs) noexcept and friend
constexpr bool operator!=(const bigint& lhs, const bigint& rhs) noexcept and
have them use the existing Compare(const bigint& lhs, const bigint& rhs)
noexcept (or the member Compare(rhs) const) to return true when Compare(...)
yields std::strong_ordering::equal and false otherwise; place these alongside
the existing friend operator<=> so equality is properly defined for the
variable-length bigint.
| int main(int argc, char* argv[]) { | ||
|
|
||
| std::vector<std::string> names; | ||
| // arg[1]{ 0 = server, 1 = client } | ||
|
|
||
| Packet::bytearray ToBytes() const { | ||
| Packet::bytearray ret; | ||
| Packet::StoreBytes(ret, names); | ||
| return ret; | ||
| } | ||
| std::vector<std::string> args; | ||
| args.insert(args.end(), argv, argv + argc); | ||
|
|
||
| Packet::byte_view FromBytes(Packet::byte_view view) { | ||
| Packet::LoadBytes(view, names); | ||
| return view; | ||
| if (args.size() <= 1) { | ||
| return -1; | ||
| } | ||
| }; | ||
|
|
||
| struct ContainerInVariable { | ||
| std::vector<ContainerInContainer> container; | ||
|
|
||
| Packet::bytearray ToBytes() const { | ||
| Packet::bytearray ret; | ||
| Packet::StoreBytes(ret, container); | ||
| return ret; | ||
| if (std::stoi(args[1]) == 0) { | ||
| Server(); | ||
| } | ||
|
|
||
| Packet::byte_view FromBytes(Packet::byte_view view) { | ||
| Packet::LoadBytes(view, container); | ||
| return view; | ||
| } | ||
| }; | ||
| #include "include/Cryptgraphy/KeyManager.h" | ||
|
|
||
| int main(int argc, char* argv[]) { | ||
|
|
||
| //KeyManager Keya; | ||
| //KeyManager Keyb; | ||
| // | ||
| //auto tp = std::chrono::high_resolution_clock::now(); | ||
| // | ||
| //auto kE = Keya.MakeQKey(); | ||
| //auto kF = Keyb.MakeQKey(); | ||
| // | ||
| //auto Ga = Keya.MakeSharedKey(kF); | ||
| //auto Gb = Keyb.MakeSharedKey(kE); | ||
| // | ||
| //auto ns = std::chrono::duration_cast<std::chrono::nanoseconds>(std::chrono::high_resolution_clock::now() - tp).count(); | ||
| // | ||
| //bool same = Ga == Gb; | ||
| // | ||
| //std::cout << (double)ns / 1000 / 1000 / 1000 << "s" << std::endl; | ||
| //std::cout << std::boolalpha << "shared key same: " << same << std::endl; | ||
| // | ||
| //for (auto&& b : Ga) { | ||
| // std::cout << std::hex << std::setw(2) << std::setfill('0') << std::right << (int)b; | ||
| //} | ||
| //std::cout << std::endl; | ||
| // | ||
| //for (auto&& b : Gb) { | ||
| // std::cout << std::hex << std::setw(2) << std::setfill('0') << std::right << (int)b; | ||
| //} | ||
| //std::cout << std::endl; | ||
|
|
||
| KeyManager key; | ||
| std::string message = "I have skill is write low level programing language."; | ||
|
|
||
| auto q = ECDSA::MakePublicKey(key.GetSecretKey()); | ||
|
|
||
| auto v = ECDSA::Sign(key.GetSecretKey(), {message.begin(), message.end()}); | ||
|
|
||
| bool ret = ECDSA::Verify(q, v, {message.begin(), message.end()}); | ||
|
|
||
| std::cout << "message: \"" << message << "\"" << std::endl; | ||
| std::cout << "Q: {" << q.x.value.ToString(16) << ", " << q.y.value.ToString(16) << "}" << std::endl; | ||
| std::cout << "(r, s)(bytes): "; | ||
| for (auto&& b : v) { | ||
| std::cout << std::hex << std::setw(2) << std::setfill('0') << std::right << (int)b; | ||
| else { | ||
| Client(); | ||
| } | ||
| std::cout << std::endl; | ||
|
|
||
| std::cout << std::boolalpha << ret; | ||
|
|
||
| //std::string message = "0123456789abcdef"; | ||
| //Cryptgraphy::bytearray data{message.begin(), message.end()}; | ||
| // | ||
| //auto tp = std::chrono::high_resolution_clock::now(); | ||
| // | ||
| //auto ret = SHAKE256::HasherN(data, 64); | ||
| // | ||
| //auto ns = std::chrono::duration_cast<std::chrono::nanoseconds>(std::chrono::high_resolution_clock::now() - tp).count(); | ||
| // | ||
| //std::cout << (double)ns / 1000 / 1000 << "ms" << std::endl; | ||
| //std::cout << std::boolalpha << "hash: "; | ||
| // | ||
| //for (auto&& c : ret) { | ||
| // std::cout << std::hex << std::right << std::setw(2) << std::setfill('0') << (int)c; | ||
| //} | ||
|
|
||
| //using int_t = bigint<8>; | ||
| //using modint_t = ModInt<int_t>; | ||
| //using projective_t = ECProjective<modint_t>; | ||
| // | ||
| //modint_t::Factory xmodp = "ffffffff00000001000000000000000000000000ffffffffffffffffffffffff"; | ||
| //projective_t::Factory projective = WeierstrassParameter<modint_t>( | ||
| // xmodp("ffffffff00000001000000000000000000000000fffffffffffffffffffffffc"), | ||
| // xmodp("5ac635d8aa3a93e7b3ebbd55769886bc651d06b0cc53b0f63bce3c3e27d2604b") | ||
| //); | ||
| // | ||
| //auto G = projective( | ||
| // xmodp("6b17d1f2e12c4247f8bce6e563a440f277037d812deb33a0f4a13945d898c296"), | ||
| // xmodp("4fe342e2fe1a7f9b8ee7eb4a7c0f9e162bce33576b315ececbb6406837bf51f5"), | ||
| // xmodp(1) | ||
| //); | ||
| // | ||
| //auto view = [](const std::string& name, const projective_t& p) { | ||
| // std::cout << name << ": {" | ||
| // << p.x.value.ToString(16) << ", " | ||
| // << p.y.value.ToString(16) << ", " | ||
| // << p.z.value.ToString(16) << "}" | ||
| // << std::endl; | ||
| //}; | ||
| //auto check = [](const projective_t& p) { | ||
| // auto a = p.ToAfinPoint(); | ||
| // std::cout << "Check: " << std::boolalpha | ||
| // << a.GetParam().CheckPoint(a.x, a.y) | ||
| // << std::endl; | ||
| //}; | ||
| // | ||
| //auto d = G.Double(); | ||
| // | ||
| //view("double", d); | ||
| //check(d); | ||
| // | ||
| //auto add = G.Add(d); | ||
| // | ||
| //view("add", add); | ||
| //check(add); | ||
| // | ||
| //auto scaler = G.Scaler(xmodp(100)); | ||
| // | ||
| //view("scaler", scaler); | ||
| //check(scaler); | ||
|
|
||
| // arg[1]{ 0 = server, 1 = client } | ||
|
|
||
| //std::vector<std::string> args; | ||
| //args.insert(args.end(), argv, argv + argc); | ||
| // | ||
| //if (args.size() <= 1) { | ||
| // return -1; | ||
| //} | ||
| // | ||
| //if (std::stoi(args[1]) == 0) { | ||
| // Server(); | ||
| //} | ||
| //else { | ||
| // Client(); | ||
| //} | ||
| // | ||
| //return 0; | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
std::stoi が例外をスローする可能性がある
args[1] が数値でない文字列の場合、std::stoi は std::invalid_argument をスローし、非常に大きい数値の場合は std::out_of_range をスローします。try-catch で囲むか、事前にバリデーションを行ってください。
🛡️ 修正案
- if (std::stoi(args[1]) == 0) {
- Server();
- }
- else {
- Client();
- }
+ try {
+ if (std::stoi(args[1]) == 0) {
+ Server();
+ }
+ else {
+ Client();
+ }
+ }
+ catch (const std::exception& e) {
+ std::cerr << "invalid argument: " << e.what() << std::endl;
+ return -1;
+ }🤖 Prompt for AI Agents
In `@Socket.cpp` around lines 28 - 47, The main function currently calls
std::stoi(args[1]) without validation which can throw std::invalid_argument or
std::out_of_range; wrap the conversion in a try-catch or pre-validate the string
(e.g., check digits and optional sign or use std::from_chars) and handle errors
by printing a usage/error message and returning non-zero; specifically update
main to guard the std::stoi call (or replace it) and only call Server() or
Client() when the parsed value is valid, otherwise return an error code and
avoid uncaught exceptions.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
include/Socket.h (2)
576-607:⚠️ Potential issue | 🔴 Critical
Packet参照の[&]キャプチャも同様にダングリング参照のリスクがあります。
ASyncSend(const Packet& src)とASyncEncryptionSend(const Packet& src)で[&]キャプチャを使用しています。呼び出し元がPacketを一時オブジェクトとして渡した場合（例:ASyncSend(Packet(...))）、非同期タスク実行時にsrcは既に破棄されています。
Packetをコピーキャプチャするか、呼び出し側の責任を明確にドキュメント化してください。🐛 修正案
std::future<bool> ASyncSend(const Packet& src) { - return std::async(std::launch::async, [&]() { + return std::async(std::launch::async, [this, src]() { return this->Send(src); }); }std::future<bool> ASyncEncryptionSend(const Packet& src) { - return std::async(std::launch::async, [&]() { + return std::async(std::launch::async, [this, src]() { return this->EncryptionSend(src); }); }
730-732:⚠️ Potential issue | 🟡 Minor
#undef SOCKET_H_USE_NAMESPACEがユーザー定義マクロを消去します。Line 732 で
SOCKET_H_USE_NAMESPACEを#undefしていますが、これはユーザーがインクルード前に定義するマクロです。このヘッダーをインクルードした後、同マクロが消えてしまうため、複数ヘッダーで同じマクロを参照する場合に問題になります。意図的な設計であれば問題ありませんが、影響を把握しておいてください。include/Cryptgraphy/MultiWordInt.h (2)
88-88:⚠️ Potential issue | 🔴 Critical自己ムーブ代入で use-after-free が発生します
this == &fromの場合、delete m_wordsで自身のデータを解放した後、m_words = from.m_words（既に解放済み）を代入し、最終的にfrom.m_words = nullptrでthis->m_wordsもnullptrになります。std::swapなどで自己代入が発生するケースがあります。🐛 修正案
-constexpr bigint& operator=(bigint&& from) noexcept { delete m_words; m_words = from.m_words; from.m_words = nullptr; return *this; } +constexpr bigint& operator=(bigint&& from) noexcept { + if (this != &from) { + delete m_words; + m_words = from.m_words; + from.m_words = nullptr; + } + return *this; +}
34-35:⚠️ Potential issue | 🟠 Majorムーブ後オブジェクトの
words()アクセスでヌルポインタ参照が発生しますムーブコンストラクタ（Line 35）およびムーブ代入（Line 88）の後、移動元の
m_wordsはnullptrになります。移動元オブジェクトに対するwords()、bits()、Compare等の呼び出しはヌルポインタ参照となります。
words()にアサーションを追加するか、ムーブ後に空のarr_tを再確保することを検討してください。🛡️ 修正案（防御的アプローチ）
-constexpr bigint(bigint&& from) noexcept : m_words(nullptr) { m_words = from.m_words; from.m_words = nullptr; } +constexpr bigint(bigint&& from) noexcept : m_words(from.m_words) { from.m_words = new arr_t(); }もしくは
words()でヌルチェック：-constexpr arr_t& words() noexcept { return *m_words; } +constexpr arr_t& words() noexcept { assert(m_words && "Accessing moved-from bigint"); return *m_words; }
🤖 Fix all issues with AI agents
In `@include/Cryptgraphy/MultiWordInt.h`:
- Around line 398-399: The subspan call is selecting the upper half instead of
the lower half, so xl/yl are passed as zeros to the recursive add; change
arr_view(t[0].words()).subspan(halfwords) and
arr_view(t[1].words()).subspan(halfwords) to select the lower half (e.g. use
.first(halfwords) or .subspan(0, halfwords)) so that xl/yl refer to the low half
where FromWords(xl) += FromWords(xh) stores its result (references: xl, yl, xh,
yh, arr_view, subspan, FromWords, t[0], t[1]).
In `@include/Socket.h`:
- Around line 524-535: Recv() (and EncryptionRecv()) allocates a buffer using
head.Size directly, which is untrusted network data and can cause OOM; add a
validation step after constructing Header head (from headbuf) to check head.Size
against a defined maximum allowed packet size (e.g., MAX_PACKET_SIZE constant or
config) and return std::nullopt (or an error) if it exceeds that limit or is
zero, before allocating bytearray data(head.Size); apply the same size-check
logic in EncryptionRecv() where it uses head.Size to guard buffer allocation and
any subsequent reads.
🧹 Nitpick comments (4)
include/Socket.h (2)
609-629:_Send/_Recv/_ASyncSend/_ASyncRecvが public スコープにあります。アンダースコアプレフィックス付きのメソッドは内部実装を示唆していますが、
publicセクション（Line 379 以降）に配置されています。protectedセクションに移動するか、プレフィックスを外して正式な public API として扱うことを検討してください。また、Line 625-629 の
_ASyncRecvで[this, &target]とtargetを参照キャプチャしていますが、_ASyncSend（Line 620）がtargetを値キャプチャしているのとは非対称です。出力パラメータなので参照は意図的と思われますが、呼び出し元がバッファの生存期間を保証する必要がある点を明記すべきです。
381-386:bytearrayとmemcpyableがSocketBase（Line 245-248）とbasic_TCPSocketで二重定義されています。
basic_TCPSocketはSocketBaseを継承しているため、基底クラスのbytearrayとmemcpyableがそのまま使えます。byte_viewとbyte_refのみ追加すれば十分です。♻️ 重複削除の提案
- using bytearray = SocketDetail::bytearray; using byte_view = SocketDetail::byte_view; using byte_ref = SocketDetail::byte_ref; - - template<class T> - static constexpr bool memcpyable = SocketDetail::memcpyable<T>;include/Cryptgraphy/MultiWordInt.h (2)
745-745: 固定長bigintが毎回ヒープ確保するのはパフォーマンスに影響します
arr_t* m_words = new arr_t()により、すべてのbigintインスタンス生成時にヒープ確保が行われます。特にNormalMulやKaratuba_Legacyのような関数内で大量の一時オブジェクトが生成されるため、アロケーションコストが蓄積します。
constexpr評価のための設計であると理解しますが、bits()のreinterpret_cast（Line 737）の TODO が解決できれば、直接arr_t m_words{};に変更してヒープ確保を排除できます。
753-956: 可変長bigint<0, _sign>にコンストラクタ・代入演算子・算術演算子が未実装です現状ではデフォルトコンストラクタ（
m_words{}で空ベクター）しか生成されず、以下が欠けています：
word_t/sword_tからの変換コンストラクタinitializer_list/ 文字列からのコンストラクタoperator+=,-=,*=,/=およびフリー関数版の算術演算子operator<<=,>>=およびフリー関数版のシフト演算子operator&,|,^のフリー関数版（&=,|=,^=は定義済み）Parse,ToString,ToHex等の I/O サポート意図的に段階的実装であれば問題ありませんが、現状では外部から実質利用不可能です。
このタスクの追跡用に Issue を作成しますか？
| std::optional<Packet> Recv() { | ||
| bytearray head(Packet::HeaderSize); | ||
| if (!Recv(head)) { | ||
| Packet::header_bytes headbuf{}; | ||
| if (!Recv(headbuf)) { | ||
| return std::nullopt; | ||
| } | ||
| Packet pak; | ||
| pak.SetBuffer(std::move(head)); | ||
| bytearray data(pak.GetHeader()->Size); | ||
| Header head = std::bit_cast<Header>(headbuf); | ||
| bytearray data(head.Size); | ||
| if (!Recv(data)) { | ||
| return std::nullopt; | ||
| } | ||
| return Packet(pak.GetHeader()->Type, data); | ||
| return Packet(head.Type, data); | ||
| } |
There was a problem hiding this comment.
ネットワークから受信した head.Size によるバッファ割り当てにサイズ制限がありません。
Line 530 で bytearray data(head.Size) としていますが、head.Size はネットワークから受信した生データから bit_cast で得られた値です。悪意あるピアが巨大な Size を送信した場合、OOM（メモリ不足）によるサービス拒否が発生します。Lines 557-558 の EncryptionRecv() にも同じ問題があります。
最大パケットサイズの上限チェックを追加することを推奨します。
🛡️ 修正案の例
+ static constexpr uint32_t MaxPacketSize = 16 * 1024 * 1024; // 16MB upper limit
+
std::optional<Packet> Recv() {
Packet::header_bytes headbuf{};
if (!Recv(headbuf)) {
return std::nullopt;
}
Header head = std::bit_cast<Header>(headbuf);
+ if (head.Size > MaxPacketSize) {
+ return std::nullopt;
+ }
bytearray data(head.Size);
if (!Recv(data)) {
return std::nullopt;
}
return Packet(head.Type, data);
}EncryptionRecv() にも同様のチェックを追加してください。
🤖 Prompt for AI Agents
In `@include/Socket.h` around lines 524 - 535, Recv() (and EncryptionRecv())
allocates a buffer using head.Size directly, which is untrusted network data and
can cause OOM; add a validation step after constructing Header head (from
headbuf) to check head.Size against a defined maximum allowed packet size (e.g.,
MAX_PACKET_SIZE constant or config) and return std::nullopt (or an error) if it
exceeds that limit or is zero, before allocating bytearray data(head.Size);
apply the same size-check logic in EncryptionRecv() where it uses head.Size to
guard buffer allocation and any subsequent reads.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/Packet.h (1)
265-274:⚠️ Potential issue | 🔴 Critical
GetArray<T>()でmemcpyのサイズ不整合によるバッファオーバーフローの可能性。Line 270 で
dataSizeはデータ部分のバイト数をsizeof(T)で切り捨て除算して算出されますが、Line 272 のmemcpyは切り捨て前の全バイト数 (m_buffer.size() - HeaderSize) をコピーします。データ部分のバイト数が
sizeof(T)の倍数でない場合、vectorの確保領域を超えて書き込みが発生します。例:
sizeof(T)==4、データ部分が5バイトの場合:
dataSize = 5/4 = 1→ vector は4バイト分memcpyは5バイトコピー → 1バイト超過書き込み🐛 修正案
template<class T> std::optional<std::vector<T>> GetArray() const requires (memcpyable<T> && !from_byteable<T>) { if (CheckHeader()) { return std::nullopt; } size_t dataSize = (m_buffer.size() - HeaderSize) / sizeof(T); std::vector<T> data(dataSize); - std::memcpy(data.data(), m_buffer.data() + HeaderSize, m_buffer.size() - HeaderSize); + std::memcpy(data.data(), m_buffer.data() + HeaderSize, dataSize * sizeof(T)); return data; }
🤖 Fix all issues with AI agents
In `@include/Packet.h`:
- Around line 213-224: GetRawData() and RefRawData() incorrectly use
CheckHeader() with its default option (which enforces HeaderSize+1), causing
zero-length payload packets to return nullopt; change both to call
CheckHeader(0) so only header presence is validated (refer to GetRawData,
RefRawData, CheckHeader, HeaderSize, and m_buffer when making the change).
🧹 Nitpick comments (1)
include/Packet.h (1)
403-405:CheckHeaderの命名とデフォルト引数について。
CheckHeaderはtrue= 不正（バッファ不足）を返しますが、一般的に "Check" や "Is" 系のメソッドはtrue= 正常を期待する慣例があります。IsBufferTooSmallや、反転してHasValidHeader(size_t minDataSize = 0)のような命名にすると、呼び出し側のif文の意図が明確になります。また、デフォルト引数
option = 1は上述のGetRawData等のバグの原因となっています。デフォルトを0に変更するか、データサイズ検証が必要な箇所で明示的に引数を渡す方が安全です。
ただ、この方式の非同期はおそらく使いづらいので今後使いやすいのを作る
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
include/Cryptgraphy/MultiWordInt.h (1)
743-746:⚠️ Potential issue | 🟠 Major
m_wordsを生ポインタでnewするのはconstexprのための妥協と理解しますが、リスクがあります
arr_t* m_words = new arr_t()はヒープ割り当てを行いますが、std::unique_ptrを使用できない制約がある場合でも、以下の点に注意してください：
- Line 88 の
operator=(bigint&&)に自己代入保護がありません。a = std::move(a)でdelete m_words後に無効なポインタを読みます。- コピーコンストラクタ（Line 34）は
*m_words = *from.m_wordsでデフォルトのnew arr_t()に書き込むため正しいですが、ムーブ後のfromに対してfrom.words()を呼ぶと nullptr デリファレンスになります。🛡️ 自己代入保護の修正案
constexpr bigint& operator=(bigint&& from) noexcept { + if (this == &from) return *this; delete m_words; m_words = from.m_words; from.m_words = nullptr; return *this; }include/Socket.h (1)
576-585:⚠️ Potential issue | 🟠 Major
ASyncSend(const Packet&)/ASyncRecv()のラムダが[&]でキャプチャしており、ダングリングリスクがありますLine 577 と 582 で
[&]キャプチャを使用しています。byte_view版（Line 566）では[=]に修正済みですが、Packet版は未修正です。
ASyncSend(const Packet&)では、呼び出し元がfutureを待つ前にPacketがスコープを抜ける可能性があります。thisのキャプチャは問題ありませんが、srcの参照キャプチャは危険です。🐛 修正案
std::future<bool> ASyncSend(const Packet& src) { - return std::async(std::launch::async, [&]() { + return std::async(std::launch::async, [this, src]() { return this->Send(src); }); }Line 598-601 の
ASyncEncryptionSend(const Packet&)にも同じ問題があります。
🤖 Fix all issues with AI agents
In `@include/Cryptgraphy/MultiWordInt.h`:
- Around line 320-365: The Karatuba_Legacy implementation incorrectly calls
Karatuba for recursive multiplies (z0, z2, z1) which pulls in the buggy
shared-state Karatuba; update the three recursive calls inside Karatuba_Legacy
so they call Karatuba_Legacy(xl, yl), Karatuba_Legacy(xh, yh), and
Karatuba_Legacy(xl, yl) respectively (replace references to Karatuba with
Karatuba_Legacy) to ensure pure value-based recursion and correct behavior.
- Around line 214-226: FromWords currently copies elements from the input range
r into the bigint's words() but stops early when r is exhausted, leaving higher
words unchanged; update FromWords to zero-clear any remaining elements of
words() after the copy (e.g., using std::fill/std::fill_n or a loop) so that
bigint::FromWords(R&& r) guarantees higher words are set to 0 when r has fewer
elements — adjust the body of bigint::FromWords (the loop over words() and the
post-copy handling) to perform that zero-fill; keep the existing signature and
constraints intact so calls like z0.FromWords(words) produce a fully zeroed high
part.
In `@include/Socket.h`:
- Around line 549-550: The current logic sends the header in plaintext
(Send(head)) and then dereferences *src.GetRawData() into EncryptionSend(),
leaking packet type/size and risking a nullptr when payload is absent; update
the send path so EncryptionSend(Packet) covers both header and payload (or
explicitly include the header bytes in the encrypted buffer) and avoid directly
dereferencing GetRawData()—instead call GetRawData() safely (check std::optional
before use) or use a GetRawData variant consistent with CheckHeader() (e.g.,
pass the same option flag) so header-only packets don't blow up; adjust the
Send(head) / EncryptionSend usage and add a presence check for GetRawData()
where Send(head) and EncryptionSend are combined.
🧹 Nitpick comments (2)
include/Socket.h (2)
565-569:ASyncSend(byte_view)の[=]キャプチャ —byte_viewはstd::spanのため、参照先バッファのライフタイムに注意
byte_view（std::span）は値コピーされますが、参照先のバッファ自体はコピーされません。呼び出し元が非同期タスク完了前にバッファを解放すると、ダングリングアクセスになります。これはAPI設計上の注意事項であり、ドキュメントに明記することを推奨します。
ASyncEncryptionRecv(byte_ref)(Line 592) も同様です。
609-629:_Send/_Recvテンプレートヘルパーは便利ですが、命名規約の懸念
_プレフィックスは C++ 標準で予約されている名前と衝突する可能性があります（グローバルスコープではアンダースコア＋大文字は予約）。クラスメンバーとしては技術的に問題ありませんが、一般的に_プレフィックスは避けることが推奨されます。
include/Cryptgraphy/MultiWordInt.h
Outdated
| static constexpr bigint Karatuba_Legacy(const bigint& x, const bigint& y) { | ||
| bigint ret = 0; | ||
|
|
||
| return *this; | ||
| if (x == 0 || y == 0) { | ||
| return ret; | ||
| } | ||
|
|
||
| count_t nbit = std::max(x.GetNBit(), y.GetNBit()); | ||
| count_t halfbits = (nbit + (nbit & 1)) / 2; | ||
|
|
||
| if (halfbits <= WordBits * 2) { | ||
| ret = NormalMul(x, y); | ||
| return ret; | ||
| } | ||
|
|
||
| bigint halfmask = (bigint(1) << halfbits) - 1; | ||
|
|
||
| bigint xl = x; | ||
| bigint xh = x; | ||
| bigint yl = y; | ||
| bigint yh = y; | ||
|
|
||
| xl &= halfmask; | ||
| xh >>= halfbits; | ||
| yl &= halfmask; | ||
| yh >>= halfbits; | ||
|
|
||
| bigint z0 = Karatuba(xl, yl); | ||
| bigint z2 = Karatuba(xh, yh); | ||
|
|
||
| xl += xh; | ||
| yl += yh; | ||
| bigint z1 = Karatuba(xl, yl); | ||
|
|
||
| z1 -= z0; | ||
| z1 -= z2; | ||
| ret += z0; | ||
|
|
||
| z1 <<= halfbits; | ||
| z2 <<= (2 * halfbits); | ||
|
|
||
| ret += z1; | ||
| ret += z2; | ||
|
|
||
| return ret; | ||
| } |
There was a problem hiding this comment.
Karatuba_Legacy の実装は正しい Karatsuba アルゴリズムです。
各再帰レベルで独自のローカル変数を持ち、値渡しで結果を返すため、Karatuba（Line 366）のような共有状態の問題がありません。ただし一点：Line 347 で Karatuba_Legacy ではなく Karatuba を再帰呼び出ししています。Karatuba には前述の致命的なバグがあるため、意図通りに動作しません。
🐛 修正案：再帰呼び出しを `Karatuba_Legacy` に統一
- bigint z0 = Karatuba(xl, yl);
- bigint z2 = Karatuba(xh, yh);
+ bigint z0 = Karatuba_Legacy(xl, yl);
+ bigint z2 = Karatuba_Legacy(xh, yh);
xl += xh;
yl += yh;
- bigint z1 = Karatuba(xl, yl);
+ bigint z1 = Karatuba_Legacy(xl, yl);🤖 Prompt for AI Agents
In `@include/Cryptgraphy/MultiWordInt.h` around lines 320 - 365, The
Karatuba_Legacy implementation incorrectly calls Karatuba for recursive
multiplies (z0, z2, z1) which pulls in the buggy shared-state Karatuba; update
the three recursive calls inside Karatuba_Legacy so they call
Karatuba_Legacy(xl, yl), Karatuba_Legacy(xh, yh), and Karatuba_Legacy(xl, yl)
respectively (replace references to Karatuba with Karatuba_Legacy) to ensure
pure value-based recursion and correct behavior.
| auto head = std::bit_cast<Packet::header_bytes>(*src.GetHeader()); | ||
| return Send(head) && EncryptionSend(*src.GetRawData()); |
There was a problem hiding this comment.
EncryptionSend(Packet) でヘッダーは暗号化せず、データのみ暗号化する設計は意図的ですか？
Line 549-550 でヘッダーは平文で送信し、GetRawData() のみ暗号化しています。ヘッダーにはパケットタイプとサイズが含まれるため、ネットワーク上でこれらのメタデータが漏洩します。
また GetRawData() は std::optional を返しますが、ここで *src.GetRawData() と直接デリファレンスしています。CheckHeader() が先に呼ばれているため通常は安全ですが、CheckHeader() は option=1（デフォルト）を使用し、GetRawData() は CheckHeader(0) を使用しているため、ヘッダーのみでデータが空のパケットの場合に不整合が生じ得ます。
🤖 Prompt for AI Agents
In `@include/Socket.h` around lines 549 - 550, The current logic sends the header
in plaintext (Send(head)) and then dereferences *src.GetRawData() into
EncryptionSend(), leaking packet type/size and risking a nullptr when payload is
absent; update the send path so EncryptionSend(Packet) covers both header and
payload (or explicitly include the header bytes in the encrypted buffer) and
avoid directly dereferencing GetRawData()—instead call GetRawData() safely
(check std::optional before use) or use a GetRawData variant consistent with
CheckHeader() (e.g., pass the same option flag) so header-only packets don't
blow up; adjust the Send(head) / EncryptionSend usage and add a presence check
for GetRawData() where Send(head) and EncryptionSend are combined.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@include/Socket.h`:
- Around line 624-629: _ASyncRecv currently captures target by reference ([this,
&target]) causing potential dangling references if the caller's target goes out
of scope before the async task completes; change the implementation to avoid
referencing caller-owned storage: either capture a copy of the buffer by value
(e.g., capture a move/copy of target similar to _ASyncSend) and call _Recv on
that copy, or change the API to accept a std::shared_ptr<T> (update the template
and call site) and capture that shared_ptr by value so lifetime is extended;
update references to _Recv accordingly and adjust callers to pass the
copied/made-shared object.
🧹 Nitpick comments (4)
include/Cryptgraphy/MultiWordInt.h (2)
377-379:AssignMulは常にNormalMulを使用 —Karatubaの活用を検討
Karatubaが実装されていますが、AssignMulは常にNormalMulを使用しています。大きなワード数でのパフォーマンスが重要な場合、閾値に基づいてKaratubaに切り替える仕組みを検討してください。♻️ 提案例
constexpr bigint& AssignMul(bigint src) { - return *this = NormalMul(*this, src); + constexpr count_t KaratsubaThreshold = 4; // Words threshold + if constexpr (Words >= KaratsubaThreshold) { + return *this = Karatuba(*this, src); + } else { + return *this = NormalMul(*this, src); + } }
222-237:const auto&&は正しく動作しますが、const auto&で明確にしましょうLine 227:
const auto&&はフォワーディング参照ではありませんが、参照の畳み込み規則により lvalue 反復子でも正しくコンパイルされます。ただし、rvalue を扱うときに move セマンティクスを無効化するため、読み取り専用のアクセスが意図されている場合はconst auto&を使用して意図をより明確にしてください。🔄 推奨される変更
template<std::ranges::range R> requires (std::is_convertible_v<word_t, std::ranges::range_value_t<R>>) constexpr bigint& FromWords(R&& r) { auto beg = m_words->begin(); auto end = m_words->end(); - for (const auto&& elem : r) { + for (const auto& elem : r) { if (beg == end) { break; } *(beg++) = elem; } for (; beg != end; ++beg) { *beg = 0; } return *this; }include/Socket.h (2)
565-607: 非同期ラムダで[&]の暗黙キャプチャが使われています —[this]を明示してくださいLines 566, 577, 582, 588, 599, 604 で
[&]または[&, target = ...]が使われています。srcはムーブ済みのため直接のダングリングリスクはありませんが、[&]による暗黙のthisキャプチャは意図を不明確にします。[this, target = std::move(src)]や[this]を使用して明示的にしてください。特に Lines 582, 604 の
[&]()はthisのみが必要なので[this]で十分です。♻️ 修正案（一部）
std::future<bool> ASyncSend(bytearray&& src) { - return std::async(std::launch::async, [&, target = std::move(src)]() { + return std::async(std::launch::async, [this, target = std::move(src)]() { return this->Send(target); }); } std::future<std::optional<Packet>> ASyncRecv() { - return std::async(std::launch::async, [&]() { + return std::async(std::launch::async, [this]() { return this->Recv(); }); }他の
ASyncSend(Packet&&),ASyncEncryptionSend,ASyncEncryptionRecv()にも同様に適用してください。
609-629:_Send/_Recv/_ASyncSend/_ASyncRecv:memcpyableテンプレートヘルパー — 命名の検討機能的には正しく便利なヘルパーですが、先頭アンダースコア付きの名前（
_Send等）は C++ では予約名に近い印象を与えます（グローバルスコープではアンダースコア＋大文字は予約）。クラススコープでは技術的に合法ですが、SendValue/RecvValueのような名前の方が慣習に沿います。
| template<class T> | ||
| std::future<bool> _ASyncRecv(T& target) requires (memcpyable<T>) { | ||
| return std::async(std::launch::async, [this, &target]() { | ||
| return this->_Recv<T>(target); | ||
| }); | ||
| } |
There was a problem hiding this comment.
_ASyncRecv で target を参照キャプチャ — 呼び出し元のライフタイム管理が必要
Line 626: [this, &target] で target を参照キャプチャしていますが、非同期タスクの完了前に呼び出し元の target 変数がスコープを抜けるとダングリング参照になります。_ASyncSend (Line 620) のように値キャプチャが望ましいですが、_Recv はバッファへの書き込みが目的のため、ドキュメントで呼び出し元にライフタイム保証を要求するか、std::shared_ptr ラッパーの使用を検討してください。
🤖 Prompt for AI Agents
In `@include/Socket.h` around lines 624 - 629, _ASyncRecv currently captures
target by reference ([this, &target]) causing potential dangling references if
the caller's target goes out of scope before the async task completes; change
the implementation to avoid referencing caller-owned storage: either capture a
copy of the buffer by value (e.g., capture a move/copy of target similar to
_ASyncSend) and call _Recv on that copy, or change the API to accept a
std::shared_ptr<T> (update the template and call site) and capture that
shared_ptr by value so lifetime is extended; update references to _Recv
accordingly and adjust callers to pass the copied/made-shared object.
ルックアップテーブルに誤りがあったのを訂正
1 participant
Summary by CodeRabbit
リリースノート
New Features
Refactor
Chore