Issue #1234 - Remove 'CString' From 'ToLowerString()'

src/plugin/PluginUtil.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 35 matching lines @@
 std::wstring GetLocationUrl(IWebBrowser2& browser)
 {
   ATL::CComBSTR locationUrl;
   if (FAILED(browser.get_LocationURL(&locationUrl)) || !locationUrl)
   {
     return std::wstring();
   }
   return std::wstring(locationUrl, locationUrl.Length());
 }
 
-/**
- * \par Implementation Note
- *   This function '_wcslwr()' uses the current locale to determine case.

[sergei, 2015/12/16 10:32:44]

I don't think we need this line because it's already communicated in the header.

[Eric, 2015/12/16 13:53:26]

It's not the same. The header says is it uses the current locale. This note says
_how_ is uses the current locale. It's the difference between specification and
implementation. The specification says what the function has to do. The
implementation note says how it does it.

When you audit the code, you can dig through the documentation to find the place
that says that '_wcslwr_s()' uses the current locale, or you can just trust the
comment.

- *   This is the same function used within 'CStringW::MakeLower()', which this function replaces.

[sergei, 2015/12/16 10:32:44]

The comment on this line is not necessary to be in the code, it can be in the
commit message. However, pay attention that `CStringW::MakeLower` finally calls
`_wcslwr_s` not `_wcslwr`.

[Eric, 2015/12/16 13:53:26]

I wavered on this one. I had decided to leave it in because of the slim
possibility that we'd be introducing defects if we change the implementation
method. It's out.

Incidentally, the ATL code base, upon further examination, contains both
'_wcslwr' and '_wcslwr_s', in different functions with the same name. I had
overlooked the second function and didn't see the overload resolution.

- */
 std::wstring ToLowerString(const std::wstring& s)
 {
-  std::wstring lower(s); // copy the argument
+  std::wstring lower(s); // Copy the argument
   /*
-   * C++11 provides that 'c_str()' returns the internal array in which the string holds its value.
-   * Thus we can modify in-place after casting away the 'const' modifier from 'c_str()'.
-   *
-   * C++11 also provides that 'c_str()' return a null-terminated string.
-   * On the other hand, '_wcslwr_s' throws an assertion failure saying that it's not null-terminated.
-   * So we're using the "insecure" version and suppressing the deprecation warning.

[sergei, 2015/12/16 10:32:44]

Although "insecure" version might work good because Microsoft had cared about it
already, I have a strong opinion that we should use "secure" version.

Pay attention that the current code works fine, so I would propose to use the
same trick here. We can construct `std::wstring lower(s.c_str());` which drops
the rest of the string in case there is null character in the middle of `s`
string and converts only the beginning of the string not causing any assertion
failure.
We should reflect it in the comment "May not lower-case entire string..." and
accordingly adjust the tests.

So, I would drop that comment and do something like

// Ensure the string is null-terminated string without zero characters in the
middle because otherwise _wcslwr_s fails.
std::wstring lower(s.c_str());
assert(s.length() == lower.length() && "The argument string will be truncated
till zero value character");
errno_t err = ::_wcslwr_s(&lower[0], lower.size() + 1);
assert(err == 0 && "Cannot lower string");
if (err == 0) {
  return std::wstring();
}
return lower;

[Eric, 2015/12/16 13:53:26]

I had tried it once and it had failed. It turns out that was because the
documentation for '_wcslwr_s' is incorrect on an important point. Regarding
parameter validation, it says "length of string" where it ought to say "length
of buffer" (the one containing the string).
There's no need for special handling for embedded null. Since we never see it,
it's just bloat to take care of it specially.

I did change the commit message to something more useful, essentially a form of
"don't do that".

+   * Documentation for '_wcslwr_s' https://msdn.microsoft.com/en-us/library/y889wzfw.aspx
+   * This documentation is incorrect on an important point.
+   * Regarding parameter validation, it says "length of string" where it should say "length of buffer".
+   * The call below has argument "length + 1" to include the terminating null character in the buffer.
    */
-#pragma warning (push)
-#pragma warning(disable : 4996)
-  _wcslwr(const_cast<wchar_t*>(lower.data()));

[sergei, 2015/12/16 10:32:45]

What about returning of an empty string in case of error?

[Eric, 2015/12/16 13:53:26]

The tests aren't returning an error code, now that I understand how '_wcslwr_s'
*actually* validates parameters. With the new patch set, '_wcslwr_s' will always
succeed.

[sergei, 2015/12/16 15:53:16]

Still I would like to check the return value and have an `assert` here. However
it might be better to return the original string instead of an empty string in
case of an error.

Our tests merely test whether it works as expected with a couple of common cases
and with a couple of uncommon cases (what is the value of current locale?),
there are even no tests with non-ASCII characters with different locales, so
they don't actually cover many possible cases, so they cannot be a prove that we
don't need to take into account the return value.
Usually such conversion functions fail on a sequence of bytes which is an
invalid UTF-16 string, e.g. "0xD900 0xD900", even if it does not fail now it can
fail with some locale or start to fail after some update of the locale, and of
course it can fail if there is no enough memory, though it might be very
unlikely.

[Eric, 2015/12/16 16:38:06]

Don't need to. According to the documentation, it can't return an error
condition. There's only two conditions where it can: 

| If str is not a valid null-terminated string, the invalid parameter handler is
invoked, as described in Parameter Validation . If execution is allowed to
continue, the functions return EINVAL and set errno to EINVAL.

We're passing the function the address from 'c_str()', which guarantees a
null-terminated string.

| If numberOfElements is less than the length of the string, the functions also
return EINVAL and set errno to EINVAL.

I quote this elsewhere, because it should say "buffer" and not "string", but
we're passing it the correct number of elements now.

In other words, neither of the error conditions can occur. Therefore I decided
that not checking the error code was fine.

However, I'm not averse to defensive programming with respect to a Microsoft
library function. I've added an error check and throw 'logic_error' if it fails.
If it fails, we have an implementation defect, so throwing 'logic_error' is the
right behavior, and masking the failure is the wrong one. If we ever see such an
error, we'll need to re-implement this function, probably not using '_wcslwr_s'.

-#pragma warning (pop)
+  auto e = _wcslwr_s(const_cast<wchar_t*>(lower.c_str()), lower.length() + 1); // uses the current locale
+  if (e != 0)
+  {
+    throw std::logic_error("Error code returned from _wcslwr_s()");
+  }
   return lower;
 }