Skip to content
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.

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

Some of icompare overloads are error prone #4558

Open
teksturi opened this issue May 16, 2024 · 1 comment
Open

Some of icompare overloads are error prone #4558

teksturi opened this issue May 16, 2024 · 1 comment
Labels

Comments

@teksturi
Copy link
Contributor

Describe the bug

RemoteSyslogChannel "LOG_" and "SYSLOG_" properties cannot work. (Not tested looking
through code.)

We have following code.

void RemoteSyslogChannel::setProperty(const std::string& name, const std::string& value)
{
    ...
   
		std::string facility;
		if (Poco::icompare(value, 4, "LOG_") == 0)
			facility = Poco::toUpper(value.substr(4));
		else if (Poco::icompare(value, 7, "SYSLOG_") == 0)
			facility = Poco::toUpper(value.substr(7));
		else
			facility = Poco::toUpper(value);

Looks valid. But there is catch. icompare has following overloads

int Foundation_API icompare(const std::string& str, std::string::size_type pos, const std::string::value_type* ptr);
int Foundation_API icompare(const std::string& str1, std::string::size_type n, const std::string& str2);

So second parameter is pos and not n. So basically this has never worked? This
is only place where this pointer icompare overload is actually used. So what to
do to fix this. do we change pos to n or remove deprecate that function first and
remove it later? I can of course just fix RemoteSyslogChannel and leave this but
would love to fix problem which leads to this.

Please add relevant environment information:

  • Bug seems to be in over 13 years.
@teksturi teksturi added the bug label May 16, 2024
@teksturi
Copy link
Contributor Author

teksturi commented May 16, 2024

Ok I also found something else strange of icompare.

This does not return 0 with size of 0.

template <class S>
int icompare(
	const S& str,
	typename S::size_type pos,
	typename S::size_type n,
	const typename S::value_type* ptr)
{
        // I would put zero check here before ptr check as it is valid even with nullptr if size is 0

	poco_check_ptr (ptr);
	typename S::size_type sz = str.size();
	if (pos > sz) pos = sz;
	if (pos + n > sz) n = sz - pos;
	typename S::const_iterator it  = str.begin() + pos;
	typename S::const_iterator end = str.begin() + pos + n;
	while (it != end && *ptr)
	{
		typename S::value_type c1(static_cast<typename S::value_type>(Ascii::toLower(*it)));
		typename S::value_type c2(static_cast<typename S::value_type>(Ascii::toLower(*ptr)));
		if (c1 < c2)
			return -1;
		else if (c1 > c2)
			return 1;
		++it; ++ptr;
	}

	if (it == end)
		return *ptr == 0 ? 0 : -1;
	else
		return 1;
}

We have even unittests for this

void StringTest::testIcompare()
{
    ....
    std::string s4 = "cCcCc";
    std::string s5;
   
    // This is correct but internally it use next icompare prototype.
    assertTrue (icompare(s5, s4.c_str()) < 0);
}

template <class S>
int icompare(
	const S& str,
	const typename S::value_type* ptr)
{
        // This is wrong imo. With
	return icompare(str, 0, str.size(), ptr);
}

I would have guessed that this works same way as string compare or strncmp.
See https://godbolt.org/z/vqrGjxKn7

Basically this issue seems to evolve icompare discussion. There is so many ways
to fix this issue so I would like little conversion what you guys want. We probably
can agree that current interfaces are not right with icompare.

@teksturi teksturi changed the title RemoteSyslogChannel setProperty with "LOG_" and "SYSLOG_" not working Some of icompare overloads are error prone May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant