Parsing of incomplete rules #159

TadeasKucera · 2021-05-18T10:46:48Z

This PR solves issue #45 and allows parsing of:

incomplete rules (we can get TokenStream after parsing)
rules containing unknown identifiers (we can get text, text_formatted and condition)
YARA files importing non-existent files (we can get text, text_formatted and condition)

…ymbol out of it.

… add new such attribute.

…w ArraySymbol.

…reSymbol.

… new DictionarySymbol.

…onSymbol.

TadeasKucera · 2021-05-20T08:33:58Z

include/yaramod/types/symbol.h

+		: _type(type), _name(name), _documentation(documentation), _dataType(dataType)
+	{
+	}
+	/// @}


I needed this constructor public, so I could construct a new Symbol instance in parser_driver.cpp on line 1635.

TadeasKucera · 2021-05-20T08:39:14Z

src/parser/parser_driver.cpp

@@ -12,6 +12,8 @@
 #include "yaramod/types/token_type.h"
 #include "yaramod/utils/filesystem_operations.h"

+#include <iostream>


I started printing through std::cerr instead of throwing ParserError when parsing fails in IncompleteMode.This only happens if a YARA rule is not finished (missing }, etc.). It prints the error as Warning but we can still access the TokenStream as we can see in ParseIncompleteRuleNotFinished cpp parser test.

Printing from libraries is generally bad practice. It's much better if library returns the errors/warnings to the user and they can decide what to do with them. Imagine a situation that you don't want to print to stderr or simply stderr is missing because the application is daemon. How would you know about such error/warning?

TadeasKucera · 2021-05-20T08:43:10Z

src/parser/parser_driver.cpp

+						const auto& parentTokenText = parentExpr->getSymbolToken()->getText();
+						parentExpr->setSymbol(std::make_shared<ArraySymbol>(parentTokenText, ExpressionType::Undefined));
+					}
+				}


Why so complicated? I needed the tormatted text to still look nice, which means I had to replace some Symbol base class instances with instances of particular derived classes. Also it was necessary to determine correct types of some Token instances (not everywhere).

Good thing is, that the IdExpression::setSymbol method also updates the pointer stored in the Token associated with this Symbol instance.

TadeasKucera · 2021-05-20T08:46:51Z

src/parser/parser_driver.cpp

+		else
+		{
+			std::cerr << "Warning: '" << e.getErrorMessage() << "'\nThis Warning would be an Error if not parsing in Incomplete mode." << std::endl;
+			return true;


I would appreciate any reviewer's opinien on if this printout is actually wanted. Thank you in advance.

As I said earlier, let the user handle the prints themselves. But I'd argue here that it's pointless to show something like this to an user. Incomplete mode is opt-in so user must know what they are doing.

I decided to stop printing these Warnings, thank you for the suggestion.

TadeasKucera · 2021-05-20T08:47:56Z

tests/cpp/parser_tests.cpp

+}
+
+TEST_F(ParserTests,
+ParseIncompleteRuleNotFinished) {


This is how incomplete rule can be parsed. The TokenStream instance is still accessible.

TadeasKucera · 2021-05-20T08:49:05Z

tests/python/test_parser.py

+'''
+        self.assertEqual(expected, yara_file.text_formatted)
+
+    def test_include_undefined_file_in_incomplete_mode(self):


I already had some file including tests in this file so I added it here in Python.

metthal · 2021-05-20T12:42:40Z

src/parser/parser_driver.cpp

@@ -12,6 +12,8 @@
 #include "yaramod/types/token_type.h"
 #include "yaramod/utils/filesystem_operations.h"

+#include <iostream>


Printing from libraries is generally bad practice. It's much better if library returns the errors/warnings to the user and they can decide what to do with them. Imagine a situation that you don't want to print to stderr or simply stderr is missing because the application is daemon. How would you know about such error/warning?

metthal · 2021-05-20T12:44:51Z

src/parser/parser_driver.cpp

+		else
+		{
+			std::cerr << "Warning: '" << e.getErrorMessage() << "'\nThis Warning would be an Error if not parsing in Incomplete mode." << std::endl;
+			return true;


As I said earlier, let the user handle the prints themselves. But I'd argue here that it's pointless to show something like this to an user. Incomplete mode is opt-in so user must know what they are doing.

TadeasKucera · 2021-05-20T12:57:22Z

Thank you for your comments. I removed the Warning when ParsingError for the reason you stated.

TadeasKucera added 17 commits May 14, 2021 16:11

Add ParserMode::Incomplete and three basic tests

450e5a7

Add Symbol::Type::Undefined and TokenType::Undefined

ab3bfbd

Add TODOs summarizing the most crucial problems

18c2252

Add tests to test unknown indentifiers parsing

fbf5b6a

Do not throw when Unknown module is imported.

f3699d9

Make Symbol constructor public

d35e580

ParserMode::Incomplete: When indefined identifier found, create new S…

e832a3f

…ymbol out of it.

ParserMode::Incomplete: When access unknown attribute of a structure,…

16dd368

… add new such attribute.

ParserMode::Incomplete: Unknown identifier followed by [ -> create ne…

444fe7d

…w ArraySymbol.

ParserMode::Incomplete: Unknown identifier by . -> create new Structu…

a28849a

…reSymbol.

ParserMode::Incomplete: Unknown symbol in For in Dictionary -> create…

baf66d6

… new DictionarySymbol.

Fix TYPO: ArraySymbol -> DictionarySymbol

64a667b

ParserMode::Incomplete: Unknown symbol as a function is made a Functi…

58f5931

…onSymbol.

ParserMode::Incomplete: Not throw when incomplete rule.

a26d8ff

ParserMode::Incomplete: Not throw when nonexistent include file.

c6e96e0

ParserMode::Incomplete: Fix unknown symbol in for statement.

39c62aa

Cleanup Patch - Remove printouts, comments and fixed TODOs

5f10ab8

TadeasKucera commented May 20, 2021

View reviewed changes

TadeasKucera marked this pull request as ready for review May 20, 2021 08:49

TadeasKucera requested a review from metthal May 20, 2021 08:49

metthal requested changes May 20, 2021

View reviewed changes

Do not throw nor cerr when parsing in Incomplete mode.

896be3d

TadeasKucera requested a review from metthal May 20, 2021 12:57

TadeasKucera merged commit 5c8dd98 into master May 26, 2021

TadeasKucera deleted the parsing_of_incomplete_rules branch May 26, 2021 08:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parsing of incomplete rules #159

Parsing of incomplete rules #159

TadeasKucera commented May 18, 2021 •

edited

Loading

TadeasKucera May 20, 2021

TadeasKucera May 20, 2021

metthal May 20, 2021

TadeasKucera May 20, 2021

TadeasKucera May 20, 2021

TadeasKucera May 20, 2021

metthal May 20, 2021

TadeasKucera May 20, 2021

TadeasKucera May 20, 2021

TadeasKucera May 20, 2021

metthal May 20, 2021

metthal May 20, 2021

TadeasKucera commented May 20, 2021

Parsing of incomplete rules #159

Parsing of incomplete rules #159

Conversation

TadeasKucera commented May 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TadeasKucera commented May 20, 2021

TadeasKucera commented May 18, 2021 •

edited

Loading