Skip to content

Fix rtl string qa NULL-deref crashes (test bug + source hardening) #486

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
leginee wants to merge 3 commits into
base: trunk
Choose a base branch
from
Open

Fix rtl string qa NULL-deref crashes (test bug + source hardening) #486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c564027
Fix rtl string qa NULL-deref crashes (test bug + source hardening)
leginee Jun 18, 2026
8f1ce3d
use guards only in debug mode.
leginee Jun 21, 2026
6598116
improved comment by elaborate that it has been the former test and no…
leginee Jun 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
144 changes: 123 additions & 21 deletions main/sal/qa/rtl/ostring/rtl_str.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,34 @@ namespace rtl_str

TEST_F(compare, compare_000)
{
rtl_str_compare( NULL, NULL);
// A former version of this test passed (NULL, NULL). rtl_str_compare
// documents that both strings must be null-terminated, so NULL is
// undefined behaviour (the implementation unconditionally dereferences
// both pointers).
// Instead verify the ordering-sign contract, which had no coverage:
// a value < 0 / > 0 depending on which string is "less".
rtl::OString aStr1 = "abc";
rtl::OString aStr2 = "abd";

ASSERT_TRUE(rtl_str_compare( aStr1.getStr(), aStr2.getStr()) < 0)
<< "\"abc\" must compare less than \"abd\".";
ASSERT_TRUE(rtl_str_compare( aStr2.getStr(), aStr1.getStr()) > 0)
<< "\"abd\" must compare greater than \"abc\".";
}

TEST_F(compare, compare_000_1)
{
// A former version of this test passed (validStr, NULL) which is
// undefined behaviour.
// Verify the empty-string boundary instead (the valid analogue of an
Comment thread
leginee marked this conversation as resolved.
// "absent" string): a non-empty string is greater than the empty one.
rtl::OString aStr1 = "Line must be equal.";
rtl_str_compare( aStr1.getStr(), NULL);
rtl::OString aEmpty = "";

ASSERT_TRUE(rtl_str_compare( aStr1.getStr(), aEmpty.getStr()) > 0)
<< "a non-empty string must be greater than the empty string.";
ASSERT_TRUE(rtl_str_compare( aEmpty.getStr(), aStr1.getStr()) < 0)
<< "the empty string must be less than a non-empty string.";
}
TEST_F(compare, compare_001)
{
Expand Down Expand Up @@ -79,13 +100,26 @@ namespace rtl_str

TEST_F(compareIgnoreAsciiCase, compare_000)
{
rtl_str_compareIgnoreAsciiCase( NULL, NULL);
// Former test passed (NULL, NULL) -> undefined behaviour. Verify the
// case-insensitive ordering-sign contract instead.
rtl::OString aStr1 = "abc";
rtl::OString aStr2 = "ABD";

ASSERT_TRUE(rtl_str_compareIgnoreAsciiCase( aStr1.getStr(), aStr2.getStr()) < 0)
<< "\"abc\" must compare less than \"ABD\" ignoring case.";
ASSERT_TRUE(rtl_str_compareIgnoreAsciiCase( aStr2.getStr(), aStr1.getStr()) > 0)
<< "\"ABD\" must compare greater than \"abc\" ignoring case.";
}

TEST_F(compareIgnoreAsciiCase, compare_000_1)
{
// Former test passed (validStr, NULL) -> undefined behaviour.
// Verify the empty-string boundary instead.
rtl::OString aStr1 = "Line must be equal.";
rtl_str_compareIgnoreAsciiCase( aStr1.getStr(), NULL);
rtl::OString aEmpty = "";

ASSERT_TRUE(rtl_str_compareIgnoreAsciiCase( aStr1.getStr(), aEmpty.getStr()) > 0)
<< "a non-empty string must be greater than the empty string.";
}
TEST_F(compareIgnoreAsciiCase, compare_001)
{
Expand Down Expand Up @@ -131,13 +165,21 @@ namespace rtl_str

TEST_F(shortenedCompareIgnoreAsciiCase_WithLength, compare_000)
{
rtl_str_shortenedCompareIgnoreAsciiCase_WithLength( NULL, 0, NULL, 0, 0);
// The _WithLength variant is length-bounded, so NULL data with length 0
// is well-defined (the loop body never runs) and must return 0.
// Keep the NULL+0 call as a regression guard, but actually assert it.
sal_Int32 nValue = rtl_str_shortenedCompareIgnoreAsciiCase_WithLength( NULL, 0, NULL, 0, 0);
ASSERT_TRUE(nValue == 0) << "zero-length comparison must return 0, even for NULL data.";
}

TEST_F(shortenedCompareIgnoreAsciiCase_WithLength, compare_000_1)
{
// First string has data, second is a zero-length string (NULL data, len 0).
// Nothing is dereferenced; the function returns the length difference.
rtl::OString aStr1 = "Line must be equal.";
rtl_str_shortenedCompareIgnoreAsciiCase_WithLength( aStr1.getStr(), aStr1.getLength(), NULL, 0, 1);
sal_Int32 nValue = rtl_str_shortenedCompareIgnoreAsciiCase_WithLength( aStr1.getStr(), aStr1.getLength(), NULL, 0, 1);
ASSERT_TRUE(nValue == aStr1.getLength())
<< "comparing against a zero-length string must yield the length difference.";
}
TEST_F(shortenedCompareIgnoreAsciiCase_WithLength, compare_001)
{
Expand Down Expand Up @@ -201,7 +243,11 @@ namespace rtl_str

TEST_F(hashCode, hashCode_000)
{
rtl_str_hashCode( NULL );
// Former test passed NULL -> getLength(NULL) dereferences NULL (UB).
// Verify the empty-string boundary: hashCode("") is defined and 0.
rtl::OString aStr1 = "";
sal_Int32 nHashCode = rtl_str_hashCode( aStr1.getStr() );
ASSERT_TRUE(nHashCode == 0) << "the hashCode of an empty string must be 0.";
}

TEST_F(hashCode, hashCode_001)
Expand Down Expand Up @@ -243,7 +289,11 @@ namespace rtl_str

TEST_F(indexOfChar, indexOfChar_000)
{
rtl_str_indexOfChar( NULL, 0 );
// Former test passed NULL -> while(*pStr) dereferences NULL (UB).
// Verify the empty-string boundary: nothing is ever found -> -1.
rtl::OString aStr1 = "";
sal_Int32 nIndex = rtl_str_indexOfChar( aStr1.getStr(), 'x' );
ASSERT_TRUE(nIndex == -1) << "searching an empty string must return -1.";
}

TEST_F(indexOfChar, indexOfChar_001)
Expand Down Expand Up @@ -279,7 +329,11 @@ namespace rtl_str

TEST_F(lastIndexOfChar, lastIndexOfChar_000)
{
rtl_str_lastIndexOfChar( NULL, 0 );
// Former test passed NULL -> getLength(NULL) dereferences NULL (UB).
// Verify the empty-string boundary instead.
rtl::OString aStr1 = "";
sal_Int32 nIndex = rtl_str_lastIndexOfChar( aStr1.getStr(), 'x' );
ASSERT_TRUE(nIndex == -1) << "searching an empty string must return -1.";
}

TEST_F(lastIndexOfChar, lastIndexOfChar_001)
Expand Down Expand Up @@ -316,13 +370,20 @@ namespace rtl_str

TEST_F(indexOfStr, indexOfStr_000)
{
rtl_str_indexOfStr( NULL, 0 );
// Former test passed (NULL, NULL) -> getLength(NULL) dereferences NULL (UB).
// Verify the empty-haystack boundary: nothing is found -> -1.
rtl::OString aStr1 = "";
sal_Int32 nIndex = rtl_str_indexOfStr( aStr1.getStr(), "x" );
ASSERT_TRUE(nIndex == -1) << "searching in an empty string must return -1.";
}

TEST_F(indexOfStr, indexOfStr_000_1)
{
// Former test passed a NULL needle -> getLength(NULL) (UB).
// Verify the empty-needle boundary: an empty search string is never found.
rtl::OString aStr1 = "Line for a indexOfStr.";
rtl_str_indexOfStr( aStr1.getStr(), 0 );
sal_Int32 nIndex = rtl_str_indexOfStr( aStr1.getStr(), "" );
ASSERT_TRUE(nIndex == -1) << "an empty search string is never found -> -1.";
}

TEST_F(indexOfStr, indexOfStr_001)
Expand Down Expand Up @@ -360,13 +421,20 @@ namespace rtl_str

TEST_F(lastIndexOfStr, lastIndexOfStr_000)
{
rtl_str_lastIndexOfStr( NULL, NULL );
// Former test passed (NULL, NULL) -> getLength(NULL) dereferences NULL (UB).
// Verify the empty-haystack boundary instead.
rtl::OString aStr1 = "";
sal_Int32 nIndex = rtl_str_lastIndexOfStr( aStr1.getStr(), "Line" );
ASSERT_TRUE(nIndex == -1) << "searching in an empty string must return -1.";
}

TEST_F(lastIndexOfStr, lastIndexOfStr_000_1)
{
// Former test passed a NULL needle -> getLength(NULL) (UB).
// Verify the empty-needle boundary: an empty search string is never found.
rtl::OString aStr1 = "Line for a lastIndexOfStr.";
rtl_str_lastIndexOfStr( aStr1.getStr(), NULL );
sal_Int32 nIndex = rtl_str_lastIndexOfStr( aStr1.getStr(), "" );
ASSERT_TRUE(nIndex == -1) << "an empty search string is never found -> -1.";
}

TEST_F(lastIndexOfStr, lastIndexOfStr_001)
Expand Down Expand Up @@ -413,7 +481,11 @@ namespace rtl_str

TEST_F(replaceChar, replaceChar_000)
{
rtl_str_replaceChar( NULL, 0, 0 );
// Former test passed NULL -> while(*pStr) dereferences NULL (UB).
// Verify the empty-string boundary: replacing in "" is a no-op.
sal_Char pStr[] = "";
rtl_str_replaceChar( pStr, 'a', 'b' );
ASSERT_TRUE(pStr[0] == 0) << "replacing in an empty string must leave it empty.";
}

TEST_F(replaceChar, replaceChar_001)
Expand All @@ -440,12 +512,21 @@ namespace rtl_str

TEST_F(replaceChar_WithLength, replaceChar_WithLength_000)
{
// Length-bounded: NULL data with length 0 never dereferences -> no-op.
// Keep the NULL+0 call as a regression guard for that tolerance.
rtl_str_replaceChar_WithLength( NULL, 0, 0, 0 );
SUCCEED() << "NULL data with zero length must be tolerated (no dereference).";
}

TEST_F(replaceChar_WithLength, replaceChar_WithLength_000_1)
{
rtl_str_replaceChar_WithLength( NULL, 1, 0, 0 );
// Former test passed (NULL, 1, ...) -> the loop runs once and
// dereferences NULL (UB). Verify the length bound instead: only the
// first nLen characters are touched, the rest are left intact.
sal_Char pStr[] = "aaaa";
rtl_str_replaceChar_WithLength( pStr, 2, 'a', 'b' );
ASSERT_TRUE(rtl::OString(pStr).equals(rtl::OString("bbaa")) == sal_True)
<< "only the first nLen characters must be replaced.";
}
TEST_F(replaceChar_WithLength, replaceChar_WithLength_001)
{
Expand All @@ -471,7 +552,11 @@ namespace rtl_str

TEST_F(toAsciiLowerCase, toAsciiLowerCase_000)
{
rtl_str_toAsciiLowerCase( NULL );
// Former test passed NULL -> while(*pStr) dereferences NULL (UB).
// Verify the empty-string boundary: lowercasing "" is a no-op.
sal_Char pStr[] = "";
rtl_str_toAsciiLowerCase( pStr );
ASSERT_TRUE(pStr[0] == 0) << "lowercasing an empty string must leave it empty.";
}

TEST_F(toAsciiLowerCase, toAsciiLowerCase_001)
Expand All @@ -496,7 +581,9 @@ namespace rtl_str

TEST_F(toAsciiLowerCase_WithLength, toAsciiLowerCase_WithLength_000)
{
// Length-bounded: NULL data with length 0 never dereferences -> no-op.
rtl_str_toAsciiLowerCase_WithLength( NULL, 0 );
SUCCEED() << "NULL data with zero length must be tolerated (no dereference).";
}

TEST_F(toAsciiLowerCase_WithLength, toAsciiLowerCase_WithLength_001)
Expand Down Expand Up @@ -524,7 +611,11 @@ namespace rtl_str

TEST_F(toAsciiUpperCase, toAsciiUpperCase_000)
{
rtl_str_toAsciiUpperCase( NULL );
// Former test passed NULL -> while(*pStr) dereferences NULL (UB).
// Verify the empty-string boundary: uppercasing "" is a no-op.
sal_Char pStr[] = "";
rtl_str_toAsciiUpperCase( pStr );
ASSERT_TRUE(pStr[0] == 0) << "uppercasing an empty string must leave it empty.";
}

TEST_F(toAsciiUpperCase, toAsciiUpperCase_001)
Expand All @@ -549,7 +640,9 @@ namespace rtl_str

TEST_F(toAsciiUpperCase_WithLength, toAsciiUpperCase_WithLength_000)
{
// Length-bounded: NULL data with length 0 never dereferences -> no-op.
rtl_str_toAsciiUpperCase_WithLength( NULL, 0 );
SUCCEED() << "NULL data with zero length must be tolerated (no dereference).";
}

TEST_F(toAsciiUpperCase_WithLength, toAsciiUpperCase_WithLength_001)
Expand Down Expand Up @@ -577,8 +670,10 @@ namespace rtl_str

TEST_F(trim_WithLength, trim_WithLength_000)
{
rtl_str_trim_WithLength(NULL, 0);
// should not GPF
// Length-bounded: NULL data with length 0 never dereferences and
// returns the resulting length 0.
sal_Int32 nLen = rtl_str_trim_WithLength(NULL, 0);
ASSERT_TRUE(nLen == 0) << "trimming a zero-length string must return 0 (and not GPF).";
}

TEST_F(trim_WithLength, trim_WithLength_000_1)
Expand Down Expand Up @@ -665,8 +760,15 @@ namespace rtl_str

TEST_F(valueOfChar, valueOfChar_000)
{
rtl_str_valueOfChar(NULL, 0);
// should not GPF
// Former test passed NULL -> the function writes through NULL (UB);
// the "should not GPF" comment was wrong, it always did.
// Verify the documented behaviour on a real buffer: writes the char
// and a terminating NUL, returns 1.
sal_Char pStr[RTL_STR_MAX_VALUEOFCHAR];
sal_Int32 nLen = rtl_str_valueOfChar(pStr, 'Z');
ASSERT_TRUE(nLen == 1) << "valueOfChar must return 1.";
ASSERT_TRUE(pStr[0] == 'Z') << "valueOfChar must write the character.";
ASSERT_TRUE(pStr[1] == 0) << "valueOfChar must NUL-terminate.";
}
TEST_F(valueOfChar, valueOfChar_001)
{
Expand Down
9 changes: 7 additions & 2 deletions main/sal/qa/rtl/ostring/rtl_string.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,13 @@ namespace rtl_string

TEST_F(getLength, getLength_000)
{
rtl_string_getLength( NULL );
// should not GPF
// Former test passed NULL; the "should not GPF" comment was wrong --
// rtl_string_getLength does "return pThis->length", so NULL dereferences
// a null pointer. Verify the empty-string boundary instead: the length
// of an empty rtl_String is 0.
rtl::OString aStr;
sal_Int32 nValue = rtl_string_getLength( aStr.pData );
ASSERT_TRUE(nValue == 0) << "the length of an empty string must be 0.";
}

TEST_F(getLength, getLength_001)
Expand Down
Loading