Skip to content
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

uninitialized strings on windows hid.c #315

Open
JosepMaJAZ opened this issue Mar 5, 2017 · 7 comments
Open

uninitialized strings on windows hid.c #315

JosepMaJAZ opened this issue Mar 5, 2017 · 7 comments

Comments

@JosepMaJAZ
Copy link

JosepMaJAZ commented Mar 5, 2017

There is an unitialized memory in windows/hid.c that can cause garbage output (and maybe even crashes).
Basically, what has been detected is that on devices that do not report some of the HidD_Getxxxx() methods (i.e. that res = FALSE) is that random text appears on serial_number, manufacturer_string and product_string, basically because it's never filled.
Also, given that we also have:
free(d->serial_number);
free(d->manufacturer_string);
free(d->product_string);
it could perfectly crash in this situation.

Additionally, I add wstr[0] = 0x0000 in case for whatever reason the call to the method returned TRUE but didn't touch the string.

@392:
tmp = (struct hid_device_info*) calloc(1, sizeof(struct hid_device_info));
+++ // Initialize all memory to zero:
+++ memset(tmp, 0, sizeof(struct hid_device_info));

And then:

@425:
+++ wstr[0] = 0x0000;
/* Serial Number */
res = HidD_GetSerialNumberString(write_handle, wstr, sizeof(wstr));

@rryan
Copy link

rryan commented Mar 5, 2017

Are you sure the memset is needed? calloc both allocates and zeros.

@JosepMaJAZ
Copy link
Author

Mmmm... Now that I read the definition on cppreference, indeed, calloc should already be doing that.

What I can assert is that with these two changes, I got the problem fixed on Mixxx. Maybe it turns out it was simply the string initialization.

@signal11
Copy link
Owner

signal11 commented Mar 7, 2017

Further, free(NULL) is perfectly valid in C.

@JosepMaJAZ
Copy link
Author

Ok, let's start again.

I was wrong with the "memset" requirement (and what it came out from that, namely the problems with free). If calloc is working as expected with visual studio, then that should not be the problem I was experiencing.

Now, there is the second change, the wstr[0] = 0x0000;
It is expected that this buffer is filled with the string if the call to the Hid method returns TRUE. Well, in my case, for this particular hardware/driver, I was getting uninitialized data (random unicode characters that most of the time looked like chinese).
When i did the test with these two changes, i could get an empty string, which is all I was trying to achieve.

@signal11
Copy link
Owner

signal11 commented Mar 7, 2017

Maybe the platform code is broken then. Can you do the same for all the strings and submit a patch?

@JosepMaJAZ
Copy link
Author

JosepMaJAZ commented Mar 7, 2017

You mean, this?

Edit: attached file

diff.txt

@signal11
Copy link
Owner

signal11 commented Mar 8, 2017

I mean submit it as a pull request, and not just for the serial number string, for all the strings. Also, put the added line after the comment (rather than before)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants