Bug #4489: ConfigFile::parseNumber<unsigned>() doesn't reject negative arguments - NFD - NDN project issue tracking system

Actions

Copy link

Bug #4489

closed

ConfigFile::parseNumber<unsigned>() doesn't reject negative arguments

Added by Davide Pesavento over 6 years ago. Updated over 4 years ago.

Status:

Closed

Priority:

Normal

Assignee:

Eric Newberry

Category:

Core

Target version:

22.02

Start date:

Due date:

% Done:

100%

Estimated time:

Description

For instance, ConfigFile::parseNumber<uint16_t> is used in the TCP and UDP factories to parse the port number. Negative port numbers are accepted instead of raising an exception.

The following test case should pass but currently fails.

BOOST_AUTO_TEST_CASE(BadPort)
{
  const std::string CONFIG = R"CONFIG(
    face_system
    {
      udp
      {
        port -1
      }
    }
  )CONFIG";

  BOOST_CHECK_THROW(parseConfig(CONFIG, true), ConfigFile::Error);
  BOOST_CHECK_THROW(parseConfig(CONFIG, false), ConfigFile::Error);
}

Related issues 1 (1 open — 0 closed)

Actions

Copy link

Updated by Junxiao Shi over 6 years ago

name::Component::fromEscapedString solved a similar problem with the following code:

long parsedType = std::strtol(input.data(), nullptr, 10);
if (parsedType < tlv::NameComponentMin || parsedType > tlv::NameComponentMax ||
    to_string(parsedType).size() != equalPos) {
  BOOST_THROW_EXCEPTION(Error("Incorrect TLV-TYPE in NameComponent URI"));
}

It converts the parsed number back to a string, and checks that this string equals the input string.

Adopting the same logic to ConfigFile::parseNumber implies that numbers written in non-canonical form, such as 06363 or +6363, would be rejected. I think it's okay to require numbers to be in canonical form.

Actions

Copy link

Updated by Davide Pesavento over 6 years ago

Junxiao Shi wrote:

Adopting the same logic to ConfigFile::parseNumber implies that numbers written in non-canonical form, such as 06363 or +6363, would be rejected.

06363 would be interpreted as octal, wouldn't it?

I think it's okay to require numbers to be in canonical form.

I think so too.

Actions

Copy link

Updated by Davide Pesavento over 5 years ago

This seems to have been fixed somewhat recently in boost, probably in version 1.68.0 (Aug 2018), although I couldn't isolate the exact commit (boost::property_tree hasn't been touched since Oct 2016).

Actions

Copy link

Updated by Eric Newberry over 4 years ago

Davide Pesavento wrote:

This seems to have been fixed somewhat recently in boost, probably in version 1.68.0 (Aug 2018), although I couldn't isolate the exact commit (boost::property_tree hasn't been touched since Oct 2016).

Strangely, this appears to occur on a Jenkins 10.15 agent with boost 1.72.0: https://jenkins.named-data.net/job/NFD/7561/OS=OSX-10.15/consoleFull

However, it does not occur on a 10.14 agent with the same version of boost: https://jenkins.named-data.net/job/NFD/7561/OS=OSX-10.14/consoleFull

Actions

Copy link

Updated by Eric Newberry over 4 years ago

Status changed from New to In Progress
Assignee set to Eric Newberry

Actions

Copy link

Updated by Eric Newberry over 4 years ago

Eric Newberry wrote:

Strangely, this appears to occur on a Jenkins 10.15 agent with boost 1.72.0: https://jenkins.named-data.net/job/NFD/7561/OS=OSX-10.15/consoleFull

However, it does not occur on a 10.14 agent with the same version of boost: https://jenkins.named-data.net/job/NFD/7561/OS=OSX-10.14/consoleFull

I just tried with Ubuntu 19.10 and Boost 1.72.0 and these test cases also failed.

Actions

Copy link

Updated by Davide Pesavento over 4 years ago

Ok. Boost probably delegates the actual parsing to some lower-level C/C++ library function, which may have accidentally changed behavior in 10.14, and reverted back to the previous behavior in 10.15. Just a guess...

Actions

Copy link