Issue #41 - Bring method of determining IE version up to date

src/shared/IE_version.cpp

 #include "IE_version.h"

[sergei, 2014/07/28 11:46:27]

I don't think it should be in shared, because it's used only by the plugin.

[Eric, 2014/07/29 12:42:25]

See #404. This code is destined to move again soon.

https://issues.adblockplus.org/ticket/404

[sergei, 2014/07/29 14:45:56]

Clear, it should be here. But why will it be moved? Let's link with the shared
library (which sounds absolutely awfully, because it's a static library and
'shared' is the name. Originally proposed 'utils' is much better).

[Eric, 2014/07/29 15:17:22]

Go read #404 again. Fixing that ticket requires a library in the build system we
do not currently have.

[sergei, 2014/07/30 10:42:04]

In #404 there is nothing about any problems with libraries. It's clear that
currently 'shared' library is not used by the installer, but we can link it.

[Eric, 2014/07/30 13:02:04]

Yes, that's because the ticket states what the problem is, not what the solution
is.

Almost all the code in the existing shared library is part of the engine/plugin
pair only and does not belong in a generally shared library. I've said this
before in review comments I would have thought you had read, but we need a new
library for #404 that shares only that which needs sharing. This registry code
is the first such. 

The other thing that needs to go in there is debugger, trace, and log support.
They're not in good enough shape in the plugin yet to easily move.

 #include "Registry.h"
+#include <cstdlib>
 
 using namespace AdblockPlus;
 
 /**
  * Internal implementation of the IE version string.
  *
  * This version throws exceptions for its errors, relying on its caller to handle them.
  *
  * Quoting http://support.microsoft.com/kb/969393:
  *   "Note The version string value for Internet Explorer 10 is 9.10.9200.16384, and the svcVersion string value is 10.0.9200.16384."
  * [EH 2014-06-20] My current version of IE 11 is reporting these values:
  *   Version 9.11.9600.17041
  *   svcVersion 11.0.9600.17041
  *
  * Warning: IE version 12 and later might not behave the same, so this function should be reviewed with each new release.

[sergei, 2014/07/28 11:46:27]

Tests should run on all machines with all IE versions. If there is something
unknown, then the test fails and we can fix it. It's impossible to handle such
information in the head and to review such peaces of the code each time.

[Eric, 2014/07/29 12:42:25]

If that's the case, the best you can get is a sanity check.

+ *
+ * \par postcondition
+ *   Return value matches regex `^[0-9]{1,2}\.$`; this is a sanity check on the format.
  */
-std::wstring IE_version_string()
+std::wstring IeVersionString()
 {
-  Registry_Key IE_key(HKEY_LOCAL_MACHINE, L"Software\\Microsoft\\Internet Explorer");
-  std::wstring version(IE_key.value_wstring(L"Version"));
+  RegistryKey ieKey(HKEY_LOCAL_MACHINE, L"Software\\Microsoft\\Internet Explorer");
+  std::wstring version(ieKey.value_wstring(L"Version"));
   /*
-    * We're expecting a version string that matches the regex "^[1-9]\.".

[sergei, 2014/07/28 11:46:27]

It's also mentioned in tests, why do we need the point at the end?

[Eric, 2014/07/29 12:42:25]

We're validating the format of the version string. This isn't anywhere near the
most stringent format test we could perform. Microsoft version numbers are
consistently dotted triples or quads. A full regex match seems overkill here.

The vast majority of cases we'll see are either (a) no IE or (b) valid IE. A
corrupt registry key is a pretty rare case, so a simple test will suffice.

-    * Since IE versions 10 and 11 use a version string that begins with "9.", this simplistic parsing method works adequately.
-    */
-  if (version[1] != '.' || version[0] < L'0' || version[0] > L'9')

[sergei, 2014/07/28 11:46:27]

strictly speaking we have to check the length of `version`. What if it's zero
length?

[Eric, 2014/07/29 12:42:25]

You're right; this is a defect, since if the registry key is corrupt, the most
likely way it will be is to have an empty value.

[sergei, 2014/07/29 14:45:56]

How can you make an assumption that the most likely it's the empty string? I
would like to say, that it can be any arbitrary string, which depends on the
fantasy of the author of the next malware.

[Eric, 2014/07/29 15:17:22]

It's beyond the scope of this class to defend against malware.

Besides, I've just posted a rewritten version of these functions to rationalize
pattern matching and length handling.

[sergei, 2014/07/30 10:42:04]

All code should be safe.

[Eric, 2014/07/30 13:02:04]

OK. That's what I'll agree with. But "safe" and "defends against malware" are
two different categories.

The reason that not checking length was a defect before (it's fixed in the
latest patch set) is that evaluating 's[1]' on a string of length 1 is undefined
behavior and may throw. If it did, that would misreport an error, at best.

+   * We're expecting a version string that matches the regex "^[1-9]\.".
+   * Since IE versions 10 and 11 use a version string that begins with "9.", this simplistic parsing method works adequately.
+   */
+  if (version.length() < 2 || version[1] != '.' || version[0] < L'0' || version[0] > L'9')
   {
     throw std::runtime_error("IE version string has unexpected format");
   }
   if (version[0] != L'9')
   {
     return version;
   }
   // Assert the major version in the "Version" value is 9.
   /*
-    * Version 9 might either be an actual version 9 or it might represent a version >= 10
-    * If the value named "svcVersion" exists, we'll report that instead.
-    */
+   * Version 9 might either be an actual version 9 or it might represent a version >= 10
+   * If the value named "svcVersion" exists, we'll report that instead.
+   */
   try
   {
-    return IE_key.value_wstring(L"svcVersion");
+    version = ieKey.value_wstring(L"svcVersion"); // throws if value not found
   }
   catch (...)
   {
+    // Assert svcVersion value not found
+    // Thus the major version is 9
     return version;
+  } 
+  // Assert major version is >= 10
+  if (version.length() < 3 || version[0] < L'0' || version[0] > L'9' || version[1] < L'0' || version[1] > L'9' || version[2] != L'.')
+  {
+    throw std::runtime_error("IE version string has unexpected format");
   }
+  return version;
 }
 
-std::wstring AdblockPlus::IE::installed_version_string()

[sergei, 2014/07/28 11:46:27]

What is the aim of this method? I think we can get rid of it. If we need it
often that let's change the original method to return an empty string in case of
problem. It might be useful to add std::error_code

[Eric, 2014/07/29 12:42:25]

It's called in engine/Main.cpp to fix #41.

[sergei, 2014/07/29 14:45:56]

If it's only the place, then do try-catch where it's called, as well I expect an
opportunity there to dump the error description into the log. Currently that
information is lost.

+std::wstring AdblockPlus::IE::InstalledVersionString()
 {
   try
   {
-    return IE_version_string();
+    return IeVersionString();
   }
   catch (...)
   {
     return L"";
   }
 }
 
-int AdblockPlus::IE::installed_major_version()
+int AdblockPlus::IE::InstalledMajorVersion()
 {
   try
   {
-    std::wstring version = IE_version_string();
+    std::wstring version = IeVersionString();
     /*
-     * If the second character is a period, we assume that the major version is a single digit.
+     * The version number is either one or two digits,
+     *   and thus either the second or third character is a period,
      */
-    if (version[ 1 ] == L'.')

[Oleksandr, 2014/07/27 22:13:30]

Nit: The array index should not be enclosed in whitespaces as well. Just
version[1] is fine. Below as well.

[sergei, 2014/07/28 11:46:27]

I would like to check the size.

[Eric, 2014/07/28 11:48:43]

I reformatted this code with Artistic Style. (It's standalone, unlike the
Eclipse formatter.) It appears that it doesn't handle white space around square
brackets. Bah.
http://astyle.sourceforge.net/

[Eric, 2014/07/29 12:42:25]

Yes; same kind of defect as above.

+    if (version[1] == L'.')
     {
-      return version[ 0 ] - L'0';
+      return version[0] - L'0';
+    } 
+    else if (version[2] == L'.')
+    {
+      return 10 * (version[0] - L'0') + (version[1] - L'0');
     }
-    // Assert IE_version_string() did not verify the syntax of the version string.
-    /*
-     * In this case, we'll assume that the version returned is two digits followed by a period.
-     * If not, we'll assume an error.
-     */
-    if (version[ 0 ] < L'0' || version[ 0 ] > L'9' || version[ 1 ] < L'0' || version[ 1 ] > L'9' || version[ 2 ] != L'.')

[sergei, 2014/07/28 11:46:27]

Is it really better than atoi?

[Eric, 2014/07/29 12:42:25]

The condition is validating the format, which we need to do anyway.

The return statement is doing the conversion. We could use atoi for it, but we'd
need to truncate the string first.

[sergei, 2014/07/29 14:45:56]

We don't need to truncate it, anyway it looks good
- only the check for the length is required
- it might be an overkill, but to have something like `int GetMajorNumber(const
std::string& version);` allows to easily test this method.

-    {
-      return 0;
-    }
-    return 10 * (version[ 0 ] - L'0') + (version[ 1 ] - L'0');
   }
   catch (...)
   {
-    return 0;
   }
+  return 0;
 }