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

Fix for error truncating registry key names #702

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@binary1248
Member

binary1248 commented Sep 24, 2014

Reported in #701.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Sep 24, 2014

Member

I've added this to my merge list. I think it's straight forward enough to be merged quickly. The changed joystick implementation in master however could still need some heavy testing.

Member

eXpl0it3r commented Sep 24, 2014

I've added this to my merge list. I think it's straight forward enough to be merged quickly. The changed joystick implementation in master however could still need some heavy testing.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Sep 24, 2014

Member

I'd still vote for just dropping the query and returning an error/default value rather than trying to read the truncated key. You might (theoretically) end up somewhere else after all.

Member

MarioLiebisch commented Sep 24, 2014

I'd still vote for just dropping the query and returning an error/default value rather than trying to read the truncated key. You might (theoretically) end up somewhere else after all.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Sep 24, 2014

Member

I'd say it depends on the handling of the other side. If there's a name withe a size greater than 255 chars, the question is, what does the OS do when writing to the registry? If it also simply truncates, then we should try to read these truncated key as well.

Member

eXpl0it3r commented Sep 24, 2014

I'd say it depends on the handling of the other side. If there's a name withe a size greater than 255 chars, the question is, what does the OS do when writing to the registry? If it also simply truncates, then we should try to read these truncated key as well.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Sep 24, 2014

Member

As far as I know key creation would simply fail. They won't get truncated (again to avoid ambiguity).

Member

MarioLiebisch commented Sep 24, 2014

As far as I know key creation would simply fail. They won't get truncated (again to avoid ambiguity).

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Sep 24, 2014

Member

Any sources to back that up?

Member

eXpl0it3r commented Sep 24, 2014

Any sources to back that up?

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Sep 24, 2014

Member

Just tried it with the following snippet (on Windows 8.1 using latest official MinGW (GCC 4.8.1)):

#include <Windows.h>

int main(int argc, char **argv){
    HKEY key;
    LONG result = RegCreateKeyA(HKEY_CURRENT_USER, "some_very_long_key_that_is_clearly_far_too_long_to_be_accepted_by_the_windows_registry_111111111111111111111111111111_22222222222222222222_3333333333333333333333333333333333_4444444444444444444444444444444444_5555555555555555555555555555_66666666666666666666666666666_777777777777777777777777777777_88888888888888888888888888888_9999999999999999999999999999999999", &key);

    if (result == ERROR_SUCCESS)
        RegCloseKey(key);

    char *msg;
    FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM, 0, result, 0, (char*)&msg, 0, 0);
    MessageBoxA(0, msg, "Test", 0);
    LocalFree(msg);
    return 0;
}

The key creation fails reporting "Wrong parameter" (localized).
If you change all underscores to backslashes (i.e. \\), they key is created, even though the absolute path is far longer than 255 characters.

So seems like this is yet another case of an outdated/unclear/wrong/misleading MSDN note:

255 characters. The key name includes the absolute path of the key in the registry, always starting at a base key, for example, HKEY_LOCAL_MACHINE.

Single keys must not be longer than 255 characters, the full path may be longer.

Member

MarioLiebisch commented Sep 24, 2014

Just tried it with the following snippet (on Windows 8.1 using latest official MinGW (GCC 4.8.1)):

#include <Windows.h>

int main(int argc, char **argv){
    HKEY key;
    LONG result = RegCreateKeyA(HKEY_CURRENT_USER, "some_very_long_key_that_is_clearly_far_too_long_to_be_accepted_by_the_windows_registry_111111111111111111111111111111_22222222222222222222_3333333333333333333333333333333333_4444444444444444444444444444444444_5555555555555555555555555555_66666666666666666666666666666_777777777777777777777777777777_88888888888888888888888888888_9999999999999999999999999999999999", &key);

    if (result == ERROR_SUCCESS)
        RegCloseKey(key);

    char *msg;
    FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM, 0, result, 0, (char*)&msg, 0, 0);
    MessageBoxA(0, msg, "Test", 0);
    LocalFree(msg);
    return 0;
}

The key creation fails reporting "Wrong parameter" (localized).
If you change all underscores to backslashes (i.e. \\), they key is created, even though the absolute path is far longer than 255 characters.

So seems like this is yet another case of an outdated/unclear/wrong/misleading MSDN note:

255 characters. The key name includes the absolute path of the key in the registry, always starting at a base key, for example, HKEY_LOCAL_MACHINE.

Single keys must not be longer than 255 characters, the full path may be longer.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Sep 24, 2014

Member

Let's face it... MSDN documentation has to be taken with a pinch of salt most of the time anyway. When they provide limits, they are what is guaranteed to work on all systems. What might work on your system (path longer than 255 characters) might not work on others, maybe because they are simply older. We don't have to push something beyond its limits just because we can 😉. The registry is already hell to work with anyway.

And more to the point, the patch I wrote only truncates a sub-key and not the whole path. So in that sense, it kind of corresponds to your findings. Sub-keys are, from what I understand, segments of the whole path anyway, and since the code already opens up the root key prior to browsing through the hierarchy using sub-keys, we don't make the assumption that the whole key length (including root) has to be smaller than 255. If we were really picky, we could split it into even more sub-keys so that longer paths are supported, but I think this would bring little benefit anyway since this is not a typical scenario. If you look at the base path that is common for all joysticks, it isn't that long either. So the only way for us to ever exceed 255 characters is for the OEM name to be abnormally long, and I think it is safe to assume that this does not happen that often either.

Member

binary1248 commented Sep 24, 2014

Let's face it... MSDN documentation has to be taken with a pinch of salt most of the time anyway. When they provide limits, they are what is guaranteed to work on all systems. What might work on your system (path longer than 255 characters) might not work on others, maybe because they are simply older. We don't have to push something beyond its limits just because we can 😉. The registry is already hell to work with anyway.

And more to the point, the patch I wrote only truncates a sub-key and not the whole path. So in that sense, it kind of corresponds to your findings. Sub-keys are, from what I understand, segments of the whole path anyway, and since the code already opens up the root key prior to browsing through the hierarchy using sub-keys, we don't make the assumption that the whole key length (including root) has to be smaller than 255. If we were really picky, we could split it into even more sub-keys so that longer paths are supported, but I think this would bring little benefit anyway since this is not a typical scenario. If you look at the base path that is common for all joysticks, it isn't that long either. So the only way for us to ever exceed 255 characters is for the OEM name to be abnormally long, and I think it is safe to assume that this does not happen that often either.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Sep 24, 2014

Member

Okay, that's an interesting way to interpret that line. :) But since we're reading only anyway, I'd really just try to do so and handle a returned error as needed. Don't preliminary truncate the path/key, since that shouldn't work anyway.

Member

MarioLiebisch commented Sep 24, 2014

Okay, that's an interesting way to interpret that line. :) But since we're reading only anyway, I'd really just try to do so and handle a returned error as needed. Don't preliminary truncate the path/key, since that shouldn't work anyway.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Sep 24, 2014

Member

This branch will be merged in a few minutes, because the current master is broken without it. If you want to keep discussing the truncation make sure to use the forum.

Member

eXpl0it3r commented Sep 24, 2014

This branch will be merged in a few minutes, because the current master is broken without it. If you want to keep discussing the truncation make sure to use the forum.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Sep 24, 2014

Member

Merged in 330ea0b

Member

eXpl0it3r commented Sep 24, 2014

Merged in 330ea0b

@eXpl0it3r eXpl0it3r closed this Sep 24, 2014

@eXpl0it3r eXpl0it3r deleted the bugfix/regkey_truncate branch Sep 24, 2014

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Sep 24, 2014

Member

Yeah, no problem. Get master fixed first.

Member

MarioLiebisch commented Sep 24, 2014

Yeah, no problem. Get master fixed first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment