Check that a CDR boolean is received as 0 or 1, if another value it i… #2489
Conversation
…s an error
* ACE/ace/CDR_Stream.cpp:
* ACE/ace/CDR_Stream.inl:
* ACE/tests/CDR_Test.cpp:
* TAO/tao/AnyTypeCode/append.cpp:
WalkthroughBoolean deserialization now validates values (only 0 or 1), propagates failures immediately when reading boolean arrays, switches a translator null-check to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
TAO/tao/AnyTypeCode/append.cpp (1)
659-801: Inconsistent error checking in TAO_Marshal_Sequence::append.Only the boolean array check (Line 676) was updated to use
!. The other array types (octet, char, short, etc.) at lines 665, 687, 698, 709, 720, 731, 742, 753, 764, 775, 786, 797 still use== 0which incorrectly interpretsfalse(failure) as success. This is inconsistent with the comprehensive fix applied toTAO_Marshal_Array::append.Apply the same fix pattern to all array types in this function:
case CORBA::tk_octet: { char* buf; if (dest->adjust (ACE_CDR::OCTET_SIZE * bounds, ACE_CDR::OCTET_ALIGN, buf) == 0) { - if (src->read_octet_array ((ACE_CDR::Octet*)buf, bounds) == 0) + if (!src->read_octet_array ((ACE_CDR::Octet*)buf, bounds)) retval = TAO::TRAVERSE_STOP; } } break;The same change should be applied to all the other array read checks (char, short, ushort, wchar, long, ulong, float, double, longlong, ulonglong, longdouble).
🧹 Nitpick comments (1)
ACE/tests/CDR_Test.cpp (1)
855-887: Well-structured test for invalid boolean detection.The test correctly validates that:
- A valid boolean (true) can be read successfully
- An invalid value (2) written as an octet fails when read as a boolean
Minor: The comment on line 871 says "stream contains 3" but the code writes value 2. Consider updating the comment for accuracy:
// Now try to extract the boolean, first should work, second should - // fail as the stream contains 3 which is a not valid value for a + // fail as the stream contains 2 which is not a valid value for a // boolean
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ACE/ace/CDR_Stream.cpp(1 hunks)ACE/ace/CDR_Stream.inl(1 hunks)ACE/tests/CDR_Test.cpp(3 hunks)TAO/tao/AnyTypeCode/append.cpp(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-02-07T16:15:05.367Z
Learnt from: likema
Repo: DOCGroup/ACE_TAO PR: 870
File: ACE/ace/UNIX_Addr.cpp:0-0
Timestamp: 2025-02-07T16:15:05.367Z
Learning: The ACE_UNIX_Addr class in ACE/ace/UNIX_Addr.cpp supports abstract paths in Linux, where paths starting with '@' are treated specially by having their first character replaced with a null terminator in the sun_path field.
Applied to files:
ACE/ace/CDR_Stream.inl
📚 Learning: 2025-07-10T08:08:10.966Z
Learnt from: jwillemsen
Repo: DOCGroup/ACE_TAO PR: 2424
File: TAO/orbsvcs/ImplRepo_Service/Locator_Loader.h:23-29
Timestamp: 2025-07-10T08:08:10.966Z
Learning: In ACE/TAO codebase, ACE_Shared_Object declares virtual init(int argc, ACE_TCHAR *argv[]) and virtual fini() methods. Classes that inherit from ACE_Shared_Object (directly or indirectly through ACE_Service_Object) should use the override specifier when overriding these methods.
Applied to files:
ACE/ace/CDR_Stream.inl
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: ubuntu-24.04 g++
- GitHub Check: ubuntu-24.04 clang++-20 c++23
- GitHub Check: ubuntu-24.04 g++-13
- GitHub Check: ubuntu-22.04 clang++-12
- GitHub Check: macos-13-C++
- GitHub Check: VS2022Release64
- GitHub Check: ubuntu-24.04 g++-11
- GitHub Check: macos-14-C++
- GitHub Check: VS2022WChar
- GitHub Check: alpine-3.18
- GitHub Check: VS2022Release32
- GitHub Check: ubuntu-24.04 g++-12
- GitHub Check: VS2022Debug64
- GitHub Check: VS2022Debug64Cxx17
- GitHub Check: ubuntu-22.04 g++-10
- GitHub Check: VS2022Debug64Cxx20
- GitHub Check: VS2022Debug32
- GitHub Check: VS2022WChar
- GitHub Check: MINGW64-gcc
- GitHub Check: windows-2022 vs2022
🔇 Additional comments (8)
TAO/tao/AnyTypeCode/append.cpp (2)
676-677: Correct fix for boolean array read error checking.The change from
== 0to!properly checks for failure. Theread_boolean_arraymethod returnstrueon success andfalseon failure, so the previous== 0check was inverted.
843-996: LGTM! Comprehensive and consistent fix for TAO_Marshal_Array::append.All array type handlers are correctly updated with:
- Explicit
{}initialization for buffer pointers- Proper failure detection using
!instead of== 0The changes correctly align with the boolean read semantics where
trueindicates success andfalseindicates failure.ACE/ace/CDR_Stream.cpp (1)
1925-1932: LGTM! Proper error propagation for boolean array reads.The change correctly propagates failures from
read_booleanup to the caller. Sinceread_booleannow validates that values are strictly 0 or 1, encountering an invalid value will causeread_boolean_arrayto returnfalseimmediately, preventing partial reads and ensuring the stream state accurately reflects the error.ACE/ace/CDR_Stream.inl (2)
748-762: LGTM! Core implementation of CORBA boolean validation.The implementation correctly enforces that CDR booleans must be 0 or 1:
- Line 758's bitwise check
!(tmp & ~1)efficiently validates thattmphas no bits set other than bit 0- Setting
good_bit_tofalsewhen an invalid value is encountered properly marks the stream as invalid- The function returns
good_bit_to propagate the validation resultThis aligns with the CORBA specification requirement for boolean encoding.
764-772: Minor style improvement.The change from
== 0to== nullptris a modern C++ style improvement for null pointer comparison.ACE/tests/CDR_Test.cpp (3)
760-762: LGTM! Consistent with new return semantics.The change from
== 0to!correctly aligns with the boolean convention wheretrueindicates success.
889-921: Comprehensive test for boolean array validation.The test effectively validates that an array containing an invalid boolean value (2) causes
read_boolean_arrayto fail, verifying the error propagation fromread_booleanthrough the array read.
1106-1110: Test invocations correctly placed.Both
boolean_test()andboolean_array_test()are invoked before the final success message, ensuring the new validation tests are executed as part of the test suite.
* TAO/tao/AnyTypeCode/append.cpp:
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ACE/tests/CDR_Test.cpp(3 hunks)TAO/tao/AnyTypeCode/append.cpp(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: macos-13-C++
- GitHub Check: ubuntu-24.04 g++-10 CodeQL
- GitHub Check: ubuntu-24.04 g++-10 CORBA/e micro
- GitHub Check: ubuntu-24.04 clang++-16
- GitHub Check: ubuntu-22.04 clang++-12
- GitHub Check: ubuntu-24.04 clang++-15
- GitHub Check: ubuntu-24.04 g++-13
- GitHub Check: ubuntu-22.04 g++-9
- GitHub Check: VS2022Debug64
- GitHub Check: VS2022Release32
- GitHub Check: VS2022Debug64Cxx20
- GitHub Check: VS2022WChar
- GitHub Check: VS2022Debug32
- GitHub Check: VS2022WChar
- GitHub Check: VS2022Release64
- GitHub Check: VS2022Debug64Cxx17
- GitHub Check: MINGW64-gcc
- GitHub Check: alpine-3.18
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: windows-2022 vs2022
- GitHub Check: ubuntu-24.04 g++
🔇 Additional comments (5)
TAO/tao/AnyTypeCode/append.cpp (2)
661-680: LGTM! Proper propagation of boolean array validation errors.The
buf {}initialization ensures the pointer starts as nullptr rather than indeterminate. The!src->read_boolean_array(...)check at line 676 will now correctly stop traversal when the underlying CDR stream detects invalid boolean values (anything other than 0 or 1), aligning with the PR objective.
854-864: LGTM! Consistent changes for array type handling.The initialization and condition check changes in
TAO_Marshal_Array::appendmirror those inTAO_Marshal_Sequence::append, ensuring that invalid boolean array values are properly rejected during array append operations as well.ACE/tests/CDR_Test.cpp (3)
760-762: LGTM! Consistent style for boolean return check.The change from
== 0to!is a stylistic improvement that aligns with how other boolean checks are written in this file and throughout the codebase.
855-887: LGTM! Good test coverage for invalid boolean validation.This test correctly verifies that:
- A valid boolean (true → 1) is read successfully
- An invalid boolean value (2 written as octet) is rejected by
read_booleanThe test structure is clear and the expected behavior aligns with the PR objective.
1106-1110: LGTM! Test invocations properly integrated.The new boolean validation tests are correctly invoked in the test sequence before the final success message.
* ACE/tests/CDR_Test.cpp:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ACE/tests/CDR_Test.cpp (1)
855-887: Consider clarifying the error message.The test logic is correct: it verifies that
read_booleanproperly rejects invalid values (value 2). However, the error message at line 882 could be more explicit about what went wrong.The current message "boolean_test bout2 failed %d" doesn't clearly convey that the test expected the read to fail but it succeeded. Consider:
- ACE_TEXT ("boolean_test bout2 failed %d\n"), bout2), + ACE_TEXT ("boolean_test: read_boolean should have rejected invalid value but succeeded with %d\n"), bout2),This makes it clearer that the test is checking for proper rejection of invalid boolean values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ACE/tests/CDR_Test.cpp(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-02-13T18:50:43.028Z
Learnt from: likema
Repo: DOCGroup/ACE_TAO PR: 2332
File: ACE/ace/Ping_Socket.cpp:227-232
Timestamp: 2025-02-13T18:50:43.028Z
Learning: In ACE framework's logging system (ACE_Log_Msg::log), `%b` is the correct format specifier for printing ssize_t values, not the standard C++ `%zd`.
Applied to files:
ACE/tests/CDR_Test.cpp
📚 Learning: 2025-02-13T18:51:40.093Z
Learnt from: likema
Repo: DOCGroup/ACE_TAO PR: 2332
File: ACE/ace/Ping_Socket.cpp:270-277
Timestamp: 2025-02-13T18:51:40.093Z
Learning: In ACE_Log_Msg logging system, '%b' is the correct format specifier to print ssize_t values, not '%zd'.
Applied to files:
ACE/tests/CDR_Test.cpp
📚 Learning: 2025-02-13T18:51:40.093Z
Learnt from: likema
Repo: DOCGroup/ACE_TAO PR: 2332
File: ACE/ace/Ping_Socket.cpp:270-277
Timestamp: 2025-02-13T18:51:40.093Z
Learning: In ACE_Log_Msg logging system, '%b' is the correct format specifier for ssize_t values, not '%zd'. This is a custom format specifier specific to ACE's logging system.
Applied to files:
ACE/tests/CDR_Test.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: ubuntu-24.04 g++-10 wchar
- GitHub Check: ubuntu-24.04 clang++-14
- GitHub Check: ubuntu-24.04 g++-12
- GitHub Check: ubuntu-24.04 g++-11
- GitHub Check: ubuntu-22.04 g++-10
- GitHub Check: macos-14-C++
- GitHub Check: macos-13-C++
- GitHub Check: VS2022Release64
- GitHub Check: VS2022Debug64
- GitHub Check: VS2022WChar
- GitHub Check: VS2022Debug32
- GitHub Check: VS2022Release32
- GitHub Check: VS2022Debug64Cxx17
- GitHub Check: VS2022WChar
- GitHub Check: VS2022Debug64Cxx20
- GitHub Check: MINGW64-gcc
- GitHub Check: alpine-3.18
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: ubuntu-24.04 g++
- GitHub Check: windows-2022 vs2022
🔇 Additional comments (3)
ACE/tests/CDR_Test.cpp (3)
760-760: LGTM: Style improvement.Using logical negation
!instead of explicit comparison with0is more idiomatic and improves readability.
889-921: LGTM: Effective test for boolean array validation.This test properly verifies that
read_boolean_arrayrejects arrays containing invalid boolean values (value 2 in position 1). The test logic is sound:
- Valid array read succeeds
- Array with invalid value correctly triggers failure
The error messages are clear and the past review feedback regarding format specifiers has been addressed.
1106-1110: LGTM: Test invocations are correctly integrated.Both new test functions are properly called from
run_mainwith appropriate error handling. The tests will execute as part of the standard CDR test suite.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
1 participant
…s an error
Summary by CodeRabbit
Bug Fixes
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.