diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index 296d006..3ee82bd 100644 --- a/.github/workflows/build-and-test.yml +++ b/.github/workflows/build-and-test.yml @@ -73,7 +73,7 @@ jobs: build_type: "Debug", cc: "clang", cxx: "clang++" } steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v5 - name: Configure CMake run: cmake -B ${{github.workspace}}/build -DCMAKE_BUILD_TYPE=${{ matrix.config.build_type }} diff --git a/.github/workflows/clang-format.yml b/.github/workflows/clang-format.yml index c16783c..16d3b46 100644 --- a/.github/workflows/clang-format.yml +++ b/.github/workflows/clang-format.yml @@ -11,12 +11,12 @@ jobs: sudo apt-get update sudo apt install clang-format-15 clang-format --version - - uses: actions/checkout@v3 + - uses: actions/checkout@v5 - name: Check format run: | find -name *.h -o -name *.cpp -o -name *.inl | xargs clang-format -i git diff - - uses: peter-evans/create-pull-request@v4 + - uses: peter-evans/create-pull-request@v8 with: token: ${{ secrets.GITHUB_TOKEN }} commit-message: "Fixes by clang-format" diff --git a/src/BinaryTokenizer.cpp b/src/BinaryTokenizer.cpp index 54b782a..b2b1c82 100644 --- a/src/BinaryTokenizer.cpp +++ b/src/BinaryTokenizer.cpp @@ -211,6 +211,8 @@ std::vector BinaryTokenizer::tokenizeArrayTagKey( std::istream& stream ) int numElements = parseInt( numElementsToken.value(), stream ); stream.seekg( streamPos ); + if ( numElements < 0 ) throw std::runtime_error( "Invalid array length." ); + std::vector arrayTokens = tokenizeArrayData( stream, numElements, typeToken.kind() ); tokens.insert( tokens.end(), arrayTokens.begin(), arrayTokens.end() ); return tokens; diff --git a/src/Parser.cpp b/src/Parser.cpp index cd3787d..a7dbe4b 100644 --- a/src/Parser.cpp +++ b/src/Parser.cpp @@ -54,17 +54,25 @@ void Parser::parse( std::istream& stream, auto it = tokens.begin(); std::string tagGroupName = ""; std::string lastName = ""; + + // Step to the next token, failing instead of moving past the end of the token list + auto next = [&it, &tokens]() + { + if ( std::distance( it, tokens.end() ) <= 1 ) throw std::runtime_error( "Unexpected end of file." ); + std::advance( it, 1 ); + }; + while ( it != tokens.end() ) { if ( it->kind() == Token::Kind::TAG ) { - std::advance( it, 1 ); + next(); assert( it->kind() == Token::Kind::NAME ); tagGroupName = parseString( *it, stream ); } else if ( isSimpleType( it->kind() ) ) { - auto simpleType = parseSimpleType( it, tagGroupName, stream ); + auto simpleType = parseSimpleType( it, tokens.end(), tagGroupName, stream ); if ( simpleType.first == "parameter.name" ) { lastName = std::get( simpleType.second ); @@ -81,12 +89,12 @@ void Parser::parse( std::istream& stream, else if ( it->kind() == Token::Kind::ARRAY ) { // Extract the type - std::advance( it, 1 ); + next(); Token typeToken = *it; assert( isSimpleType( typeToken.kind() ) ); // Extract the name - std::advance( it, 1 ); + next(); std::string name = tagGroupName + "." + parseString( *it, stream ); // Special handling for parameter tags. @@ -107,18 +115,38 @@ void Parser::parse( std::istream& stream, } // Extract number of array items - std::advance( it, 1 ); + next(); long arrayLength = parseInt( *it, stream ); + if ( arrayLength < 0 ) throw std::runtime_error( "Invalid array length." ); + + // The index of the first item + long startIndex = static_cast( std::distance( tokens.begin(), it ) ) + 1; - // Move to the first item to get the index - std::advance( it, 1 ); - long startIndex = static_cast( std::distance( tokens.begin(), it ) ); + // The array length is read from the file and must be backed by actual tokens + const long tokensAfterLength = static_cast( std::distance( it, tokens.end() ) ) - 1; + const bool isBlob = tokensAfterLength > 0 && ( it + 1 )->kind() == Token::Kind::ARRAYBLOB; - if ( it->kind() != Token::Kind::ARRAYBLOB ) + if ( isBlob ) { - // Skip rest of the array when not blob. - // The blob is always only one token and is skip already. - std::advance( it, arrayLength - 1 ); + // The blob is always only one token + std::advance( it, 1 ); + + // The data of the blob must be present in the file + stream.clear(); + const auto currentPos = stream.tellg(); + stream.seekg( 0, std::ios::end ); + const auto streamSize = stream.tellg(); + stream.seekg( currentPos ); + if ( it->end() > static_cast( streamSize ) ) + throw std::runtime_error( "Array data is incomplete." ); + } + else + { + if ( arrayLength > tokensAfterLength ) + throw std::runtime_error( "Array length exceeds the number of items in the file." ); + + // Skip rest of the array + std::advance( it, arrayLength ); } arrayInfo[name] = std::make_pair( startIndex, arrayLength ); @@ -146,12 +174,15 @@ bool Parser::isSimpleType( Token::Kind kind ) /// //-------------------------------------------------------------------------------------------------- std::pair Parser::parseSimpleType( std::vector::const_iterator& it, + std::vector::const_iterator end, const std::string& tagGroupName, std::istream& stream ) const { assert( isSimpleType( it->kind() ) ); Token::Kind kind = it->kind(); + if ( std::distance( it, end ) <= 2 ) throw std::runtime_error( "Unexpected end of file." ); + std::advance( it, 1 ); assert( it->kind() == Token::Kind::NAME ); std::string name = parseString( *it, stream ); diff --git a/src/Parser.hpp b/src/Parser.hpp index 20d3ba3..acd5adc 100644 --- a/src/Parser.hpp +++ b/src/Parser.hpp @@ -42,6 +42,7 @@ class Parser std::map>& arrayInfo ) const; std::pair parseSimpleType( std::vector::const_iterator& it, + std::vector::const_iterator end, const std::string& tagGroupName, std::istream& stream ) const; diff --git a/tests/ReaderTests.cpp b/tests/ReaderTests.cpp index 138a9a7..e3f8fb1 100644 --- a/tests/ReaderTests.cpp +++ b/tests/ReaderTests.cpp @@ -20,6 +20,7 @@ #include #include +#include #include #include @@ -310,3 +311,72 @@ TEST( ReaderTests, testReadWrongFileType ) } ASSERT_EQ( errMsg, std::string( "Unexpected file type." ) ); } + +//-------------------------------------------------------------------------------------------------- +/// The declared array length is larger than the number of items in the file. Used to read outside +/// the token list. +//-------------------------------------------------------------------------------------------------- +TEST( ReaderTests, testParseArrayLengthLargerThanContent ) +{ + auto makeFile = []( const std::string& codeNamesLength ) + { + return "roff-asc\n" + "#ROFF file#\n" + "tag filedata\n" + "int byteswaptest 1\n" + "endtag\n" + "tag parameter\n" + "char name \"composite\"\n" + "array char codeNames " + + codeNamesLength + + "\n" + " \"code name 1\"\n" + " \"code name 2\"\n" + "array int codeValues 2\n" + " 0 1\n" + "endtag\n" + "tag eof\n" + "endtag\n"; + }; + + { + std::istringstream stream( makeFile( "2" ) ); + Reader reader( stream ); + reader.parse(); + EXPECT_EQ( 2u, reader.getArrayLength( "composite.codeNames" ) ); + } + + for ( const std::string length : { "100000", "-1" } ) + { + std::istringstream stream( makeFile( length ) ); + Reader reader( stream ); + EXPECT_THROW( reader.parse(), std::runtime_error ) << "Array length " << length; + } +} + +//-------------------------------------------------------------------------------------------------- +/// Files ending in the middle of a declaration used to read outside the token list. +//-------------------------------------------------------------------------------------------------- +TEST( ReaderTests, testParseTruncatedFile ) +{ + for ( const std::string& content : { std::string( "roff-asc\n#ROFF file#\ntag" ), + std::string( "roff-asc\n#ROFF file#\ntag filedata\nint" ), + std::string( "roff-asc\n#ROFF file#\ntag filedata\nint byteswaptest" ), + std::string( "roff-asc\n#ROFF file#\ntag a\narray" ), + std::string( "roff-asc\n#ROFF file#\ntag a\narray int" ), + std::string( "roff-asc\n#ROFF file#\ntag a\narray int values" ), + std::string( "roff-asc\n#ROFF file#\ntag a\narray int values 3" ), + std::string( "roff-asc\n#ROFF file#\ntag a\narray int values 3\n 1 2" ) } ) + { + std::istringstream stream( content ); + Reader reader( stream ); + try + { + reader.parse(); + } + catch ( std::runtime_error& ) + { + // Rejecting the file is fine, reading outside the tokens is not + } + } +}