JclSysInfo causing programs freeze under Win8.1

[PizzaProgram]

Under Windows 8.1 Embedded Industrial Enterprice (Build 9600) the GetWindowsMajorVersionNumber() causing: EConvertError: '' is not a valid integer value.
It is reckless to call unsafe code from initialization part!

[ronaldhoek]

This file is now in UTF-8 encoding and should be ANSI encoding

[PizzaProgram]

I've clicked only the "Edit" here on github and edited.
It seems it has changed the encoding without my knowledge.

[ronaldhoek]

@PizzaProgram yes, that's a known thing.
You need to clone the archive to your local system and edit files locally and then push them to your personal GitBug branch.
You can still do this by cloning your GitHub branch PizzaProgram:patch-1 to your local driver and then change the encoding to ANSI again (and please check whether de special characters are correct...)

[ahausladen]

The StrToInt code in GetWindowsMajorVersionNumber was changed to StrToIntDef in August 2016.

[PizzaProgram]

@ahausladen
I've found that line:

jcl/jcl/source/common/JclSysInfo.pas

Line 4167 in 47af4fe

Result := StrToIntDef(Copy(Ver, 1, Pos('.', Ver) - 1), 2); // don't use StrBefore because it uses StrCaseMap that may not be initialized yet

That code is unsafe this way.

1. Who knows for sure, there is a point in that string ?
2. Why do we assume, there will be no characters before / after ?
   (In my experience a text should be cleaned first, leaving only numbers, before calling StrToInt.)

I haven't searched the entire unit, but I guess there are other functions like this too.
Like: GetWindowsMinorVersionNumber(), GetWindowsBuildNumber()

[PizzaProgram]

@ronaldhoek
If that code would be safe, this whole issue would not have been opened, because it would run on all types of Windows.
( ... instead of creating Access Violation error.)

[ahausladen]

This pull request fixes the symptoms by catching all exceptions. It would be better to fix the actual cause. The StrToIntDef that was added in 2016 prevents the EConvertError. So I made the assumption that the code base you used may have been too old.

About the StrToIntDef(Copy(Ver, 1, Pos('.', Ver) - 1), 2, if there is no '.' in the string, Pos('.', Ver) returns 0. And Copy(Ver, 1, -1) returns an empty string that StrToIntDef converts to the default value 2. It could be made more obvious if it would be tested for validity beforehand like:

GetWindowsMajorVersionNumber:

I := Pos('.', Ver);
if I > 0 then
  Result := StrToIntDef(Copy(Ver, 1, I - 1), 2)
else
  Result := 2;

GetWindowsMinorVersionNumber:

I := Pos('.', Ver);
if (I > 0) and (I < Length(Ver)) then
  Result := StrToIntDef(Copy(Ver, I + 1, Length(Ver)), 2)
else
  Result := 2;

I found another StrToInt call in GetWindowsBuildNumber that could throw an EConvertError if the "CurrentBuildNumber" registry value exists but isn't a number.

[PizzaProgram]

IMHO if there is no point, it should still try to convert from string to int to get the major version number.
Who knows, what will be the next version number of next windows12+ ?
12
2025

The decimal point is only a must to get the Minor version.

[PizzaProgram]

Also, my opinion stays:

• fixing the original error is nice, but:

There should be always a try - except at init.. and finalization part !

It is not fair, to write a component, that can freeze an app before it can even start.

• What if any of the Windows-calls freeze ?
• or gives back a 64bit number instead of integer?
  (Like Win10+ does, if Delphi7 app asks for "current/default printer number", if set to "automatic" -> causing a massive AV.)

[ahausladen]

I've made a commit that combines the cause-fixes and adds the try/except blocks in the InitSysInfo and FinalizeSysInfo functions.

[obones]

I believe this can now be closed?