[vwnc] Registry read issue -- can have devistating results ...

I have code which uses VW registry handling

    Win95SystemSupport  getVariableWithRoot: rootValue path: path name:
parameterName ifAbsent: errorBlock

I was reading the "Path" variable to update it.

The method allocates a 256 byte buffer to receive the result and goes
out the errorBlock which
according the comment indicates its not there.  Given that result, I
create a PATH= using the
windows default, add my stuff and put it back.

If one increases the 256 byte buffer to say 1024, it gets the Path=,
which in my case was just over
300 bytes long.

Failing due to internals like that should be a "self error:"!!!

Since it does not cost much, I suggest increasing that to 2048 or 8192
or something like that, and
making error #234 do a self error:.

--
Dennis Smith                         +1 416.798.7948
Cherniak Software Development Corporation   Fax: +1 416.798.0948
509-2001 Sheppard Avenue East        [hidden email]
Toronto, ON M2J 4Z8              sip:[hidden email]
Canada         http://www.CherniakSoftware.com
Entrance off Yorkland Blvd south of Sheppard Ave east of the DVP

_______________________________________________
vwnc mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/vwnc

There's more of those devastating results - and all of them in just two
methods. ;-)

To fix the problem in the Win95SystemSupport method, it would be better
to check if the return value of RegQueryValueExA is ERROR_MORE_DATA. If
it is, the method could retry reading the value with a larger buffer.
   http://msdn2.microsoft.com/en-us/library/ms724911.aspx
has an example for doing this properly.

The method should use RegGetValue instead of RegQueryValueExA to ensure
that the string returned has a terminating null character (or two in the
case of REG_MULTI_SZ data - but Win95SystemSupport doesn't like those
anyway), as mentioned at the MSDN page.
The current implementation does a simple
        string := buffer copyCStringFromHeap.
without checking the size of the string. The size can be accessed via
the "count" pointer after calling RegQueryValueExA. Not checking for the
size can lead to copyCStringFromHeap reading too many characters if a
registry value has no terminating null character, or if the buffer does
not have enough space for appending a null. Depending on what you do
afterwards, this can cause memory corruption. E.g., it would be a very
bad idea to replace characters in the result string.

Finally, shouldn't VisualWorks use the Unicode versions of the Windows
functions instead of the Ansi versions?

As a workaround, you could try to use the Registry package from the
public repository. It's also in the "contributed" directory of VisualWorks.

However, I suspect that Win32Value>>read in the Registry package has
different problems:

- It checks for ERROR_MORE_DATA and expands the buffer, but I think this
does not work. If I understand the MSDN documentation correctly, the
byteSizeHolder's value will be undefined in the case of ERROR_MORE_DATA,
but the method assumes that it is the real size of the data. The
Registry package definitely needs one more unit test. ;-)

- When converting the buffer to a String, the conversion omits the last
byte, probably assuming that it is null anyway. However, I think that
this assumption is wrong - it can be a valid character if the buffer
size was just one byte too small to hold a terminating null, or if the
registry value was stored without a terminating null in the first place.
So, in some cases that last character will be omitted. That's another
unit test to be written.

- The implementation in Registry converts the bytes it reads directly
into a ByteString using #changeClassTo:, without the conversion which is
done in copyCStringFromHeap. I have not yet tested what this does to
German umlauts, but it certainly looks a bit suspect.

Best regards,
Joachim Geidel

Dennis Smith schrieb am 23.02.2008 15:53:
_______________________________________________
vwnc mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/vwnc

After speculating in public about possible problems in version 23 of the
Registry package, I had to do my homework and check if I was wrong or not.

Joachim Geidel schrieb am 24.02.2008 10:08:
> However, I suspect that Win32Value>>read in the Registry package has
> different problems:
>
> - It checks for ERROR_MORE_DATA and expands the buffer, but I think this
> does not work. If I understand the MSDN documentation correctly, the
> byteSizeHolder's value will be undefined in the case of ERROR_MORE_DATA,
> but the method assumes that it is the real size of the data. The
> Registry package definitely needs one more unit test. ;-)

My suspicion was wrong. One of the unit tests covers this case already.
It writes and reads a value which is longer than the default buffer size
of Win32Value>>read. The buffer is expanded correctly. I have probably
misread the MSDN documentation. So using the Registry package is a
viable workaround for the 256 bytes limitation of
Win95SystemSupport>>getVariableWithRoot:path:name:ifAbsent: which Dennis
Smith found.

> - When converting the buffer to a String, the conversion omits the last
> byte, probably assuming that it is null anyway. However, I think that
> this assumption is wrong - it can be a valid character if the buffer
> size was just one byte too small to hold a terminating null, or if the
> registry value was stored without a terminating null in the first place.
> So, in some cases that last character will be omitted. That's another
> unit test to be written.

Buffers which are just one byte too small are handled correctly. Strings
which are not null terminated are read without their last character. I
have fixed the problem in version 24 of Registry.

> - The implementation in Registry converts the bytes it reads directly
> into a ByteString using #changeClassTo:, without the conversion which is
> done in copyCStringFromHeap. I have not yet tested what this does to
> German umlauts, but it certainly looks a bit suspect.

German umlauts are not a problem on a german Windows. I don't know
enough about character encoding issues on localized Windows to come up
with a conclusion. TwoByteStrings can't be written or read with
Registry. I suspect this should be solved by using the Unicode versions
of the registry access functions instead of their ANSI counterparts.

Best regards,
Joachim Geidel
_______________________________________________
vwnc mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/vwnc