salted-sha1-opencl: Autotune bugfixes#5889
salted-sha1-opencl: Autotune bugfixes#5889magnumripper merged 1 commit intoopenwall:bleeding-jumbofrom
Conversation
src/opencl_salted_sha_fmt_plug.c
Outdated
| new_salt.len = strlen("Hello"); | ||
| memset(&new_salt, 0, SALT_SIZE); | ||
| memcpy(new_salt.data.c, "Hello", new_salt.len); | ||
| strcpy((char*)new_salt.data.c, "Hello"); | ||
| new_salt.len = strlen((char*)new_salt.data.c); |
There was a problem hiding this comment.
Stumbled upon this by coincidence. New_salt.len would always end up as 0, screwing up the autotune.
There was a problem hiding this comment.
We generally do want to initialize the entire binary salt structs, or else comparisons for same/different salts don't work reliably. Probably irrelevant here, but maybe it'd be better to swap the old new_salt.len assignment and memset lines instead of switching to strcpy? Or switch to strncpy along with a comment that NUL padding is intentional.
There was a problem hiding this comment.
Besides salt comparisons, we may also copy salts up to SALT_SIZE, and reading uninitialized data is just not right - may be detected by various sanitizers or even hardware later on.
There was a problem hiding this comment.
This isn't in set_salt(), it's part of the autotune constructing a salt not from a test vector using its own half assed code. I guess something like set_salt(get_salt(fmt_opencl_salted_sha1.params.tests[0].ciphertext)); would do it better in one, albeit long line, lol (and something like that is what we do in the shared code, I'm pretty sure).
|
|
||
| static void create_clobj_kpc(size_t kpc) | ||
| { | ||
| global_work_size = kpc; |
There was a problem hiding this comment.
This was needed to ensure clear_keys() did not use some outdated value for gws, memsetting past buffer and b00m. I only ever saw it happen under the Test Suite, no idea why.
The ultimate problem is the format has its own autotune, not quite by our "conventions".
solardiz
left a comment
solardiz
left a comment
There was a problem hiding this comment.
Approving, but I also made a comment.
src/opencl_salted_sha_fmt_plug.c
Outdated
| new_salt.len = strlen("Hello"); | ||
| memset(&new_salt, 0, SALT_SIZE); | ||
| memcpy(new_salt.data.c, "Hello", new_salt.len); | ||
| strcpy((char*)new_salt.data.c, "Hello"); | ||
| new_salt.len = strlen((char*)new_salt.data.c); |
There was a problem hiding this comment.
We generally do want to initialize the entire binary salt structs, or else comparisons for same/different salts don't work reliably. Probably irrelevant here, but maybe it'd be better to swap the old new_salt.len assignment and memset lines instead of switching to strcpy? Or switch to strncpy along with a comment that NUL padding is intentional.
magnumripper
force-pushed
the
quickie
branch
from
ed7e486 to
b038f63
Compare
I actually tried the above and it worked right off the bat. So I pushed that fix instead! |
2 participants
Problems found while working on something else. Even segfaulted under certain conditions.