Address review comments

This commit is contained in:
Alan Griffiths 2020-04-09 10:09:23 +01:00
parent 66c0c42ee0
commit 544d8b7cfa
5 changed files with 17 additions and 19 deletions

View File

@ -20,16 +20,18 @@ namespace YAML {
* maximum depth. * maximum depth.
*/ */
class DeepRecursion : public ParserException { class DeepRecursion : public ParserException {
int m_atDepth = 0;
public: public:
// no custom dtor needed, but virtual dtor necessary to prevent slicing
virtual ~DeepRecursion() = default; virtual ~DeepRecursion() = default;
// construct an exception explaining how deep you were
DeepRecursion(int at_depth, const Mark& mark_, const std::string& msg_); DeepRecursion(int at_depth, const Mark& mark_, const std::string& msg_);
// query how deep you were when the exception was thrown // Returns the recursion depth when the exception was thrown
int AtDepth() const; int depth() const {
return m_depth;
}
private:
int m_depth = 0;
}; };
/** /**
@ -45,7 +47,6 @@ public:
*/ */
template <int max_depth = 2000> template <int max_depth = 2000>
class DepthGuard final /* final because non-virtual dtor */ { class DepthGuard final /* final because non-virtual dtor */ {
int & m_depth;
public: public:
DepthGuard(int & depth_, const Mark& mark_, const std::string& msg_) : m_depth(depth_) { DepthGuard(int & depth_, const Mark& mark_, const std::string& msg_) : m_depth(depth_) {
++m_depth; ++m_depth;
@ -54,7 +55,6 @@ public:
} }
} }
// DepthGuard is neither copyable nor moveable.
DepthGuard(const DepthGuard & copy_ctor) = delete; DepthGuard(const DepthGuard & copy_ctor) = delete;
DepthGuard(DepthGuard && move_ctor) = delete; DepthGuard(DepthGuard && move_ctor) = delete;
DepthGuard & operator=(const DepthGuard & copy_assign) = delete; DepthGuard & operator=(const DepthGuard & copy_assign) = delete;
@ -67,6 +67,9 @@ public:
int current_depth() const { int current_depth() const {
return m_depth; return m_depth;
} }
private:
int & m_depth;
}; };
} // namespace YAML } // namespace YAML

View File

@ -4,11 +4,7 @@ namespace YAML {
DeepRecursion::DeepRecursion(int at_depth, const Mark& mark_, const std::string& msg_) DeepRecursion::DeepRecursion(int at_depth, const Mark& mark_, const std::string& msg_)
: ParserException(mark_, msg_), : ParserException(mark_, msg_),
m_atDepth(at_depth) { m_depth(at_depth) {
}
int DeepRecursion::AtDepth() const {
return m_atDepth;
} }
} // namespace YAML } // namespace YAML

View File

@ -48,7 +48,7 @@ void SingleDocParser::HandleDocument(EventHandler& eventHandler) {
} }
void SingleDocParser::HandleNode(EventHandler& eventHandler) { void SingleDocParser::HandleNode(EventHandler& eventHandler) {
DepthGuard depthguard(depth, m_scanner.mark(), ErrorMsg::BAD_FILE); DepthGuard<2000> depthguard(depth, m_scanner.mark(), ErrorMsg::BAD_FILE);
// an empty node *is* a possibility // an empty node *is* a possibility
if (m_scanner.empty()) { if (m_scanner.empty()) {

View File

@ -16,7 +16,6 @@
namespace YAML { namespace YAML {
class CollectionStack; class CollectionStack;
template <int> class DepthGuard; // depthguard.h template <int> class DepthGuard; // depthguard.h
class DeepRecursion; // an exception which may be thrown from excessive call stack recursion, see depthguard.h
class EventHandler; class EventHandler;
class Node; class Node;
class Scanner; class Scanner;
@ -57,7 +56,6 @@ class SingleDocParser {
anchor_t LookupAnchor(const Mark& mark, const std::string& name) const; anchor_t LookupAnchor(const Mark& mark, const std::string& name) const;
private: private:
using DepthGuard = YAML::DepthGuard<2000>;
int depth = 0; int depth = 0;
Scanner& m_scanner; Scanner& m_scanner;
const Directives& m_directives; const Directives& m_directives;

View File

@ -1,3 +1,4 @@
#include <yaml-cpp/depthguard.h>
#include "yaml-cpp/parser.h" #include "yaml-cpp/parser.h"
#include "yaml-cpp/exceptions.h" #include "yaml-cpp/exceptions.h"
#include "mock_event_handler.h" #include "mock_event_handler.h"
@ -25,7 +26,7 @@ TEST(ParserTest, CVE_2017_5950) {
Parser parser{input}; Parser parser{input};
NiceMock<MockEventHandler> handler; NiceMock<MockEventHandler> handler;
EXPECT_THROW(parser.HandleNextDocument(handler), YAML::ParserException); EXPECT_THROW(parser.HandleNextDocument(handler), YAML::DeepRecursion);
} }
TEST(ParserTest, CVE_2018_20573) { TEST(ParserTest, CVE_2018_20573) {
@ -36,7 +37,7 @@ TEST(ParserTest, CVE_2018_20573) {
Parser parser{input}; Parser parser{input};
NiceMock<MockEventHandler> handler; NiceMock<MockEventHandler> handler;
EXPECT_THROW(parser.HandleNextDocument(handler), YAML::ParserException); EXPECT_THROW(parser.HandleNextDocument(handler), YAML::DeepRecursion);
} }
TEST(ParserTest, CVE_2018_20574) { TEST(ParserTest, CVE_2018_20574) {
@ -47,7 +48,7 @@ TEST(ParserTest, CVE_2018_20574) {
Parser parser{input}; Parser parser{input};
NiceMock<MockEventHandler> handler; NiceMock<MockEventHandler> handler;
EXPECT_THROW(parser.HandleNextDocument(handler), YAML::ParserException); EXPECT_THROW(parser.HandleNextDocument(handler), YAML::DeepRecursion);
} }
TEST(ParserTest, CVE_2019_6285) { TEST(ParserTest, CVE_2019_6285) {
@ -59,5 +60,5 @@ TEST(ParserTest, CVE_2019_6285) {
Parser parser{input}; Parser parser{input};
NiceMock<MockEventHandler> handler; NiceMock<MockEventHandler> handler;
EXPECT_THROW(parser.HandleNextDocument(handler), YAML::ParserException); EXPECT_THROW(parser.HandleNextDocument(handler), YAML::DeepRecursion);
} }