-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Undefined Behavior Sanitizer "errors" #5476
Comments
Per my review, this isn't even the only place where we end up doing NULL+0 in that file. The code looks like it's from the We might want to sync with upstream. As to NULL+0, one workaround may be to pass |
This looks easy to fix, so I will.
This looks like a real bug, but it's not obvious to me what fix is right. @magnumripper you seem to have introduced this in 4320dbe so perhaps it's yours to look into? |
The mask is 16-bit anyway, and the previous code triggered clang UbSan: racf_kdfaes_fmt_plug.c:372:23: runtime error: left shift of 238 by 24 places cannot be represented in type 'int' See openwall#5476
The mask is 16-bit anyway, and the previous code triggered clang UbSan: racf_kdfaes_fmt_plug.c:372:23: runtime error: left shift of 238 by 24 places cannot be represented in type 'int' See openwall#5476
The mask is 16-bit anyway, and the previous code triggered clang UbSan: racf_kdfaes_fmt_plug.c:372:23: runtime error: left shift of 238 by 24 places cannot be represented in type 'int' See #5476
I'm puzzled as to why this is only detected by UbSan, but not ASan. Any ideas? |
No. The interesting thing is that the error message is very clear and direct! So I tried to debug it myself, but I couldn't [1]. [1] It's a non-OpenMP build, maybe I mixed things up when I tried it. |
@claudioandre-br Please try re-enabling these tests in whatever setup you had detected the errors. These two should be fine now. Thank you! |
They are already enabled. This is the log obtained this Monday.
|
I tried adding an explicit check for this condition, but it does not trigger. clang/UbSan bug? +++ b/src/zip_fmt_plug.c
@@ -141,8 +141,12 @@ static int crypt_all(int *pcount, struct db_salt *salt)
pbkdf2_sha1_sse((const unsigned char **)pin, lens, saved_salt->salt, SALT_LENGTH(saved_salt->v.mode),
KEYING_ITERATIONS, pout, BLK_SZ, early_skip);
for (i = 0; i < MIN_KEYS_PER_CRYPT; ++i)
+{
+int ofs = 2 * key_len - late_skip;
+if (ofs + 2 > 3 * BLK_SZ) printf("ofs %d\n", ofs);
if (!memcmp(pwd_ver[i] + 2 * key_len - late_skip, saved_salt->passverify, 2))
something_hit = hits[i] = 1;
+}
if (something_hit) {
for (i = 0; i < MIN_KEYS_PER_CRYPT; ++i)
pout[i] = pwd_ver[i]; Tried the above in builds with gcc and clang, testing with In summary, 2 issues fixed, 1 more (above) looks like false positive. Closing. |
Also tried specifically |
Adding
So UbSan triggers, but my own check does not. Changing the line: if (!memcmp(pwd_ver[i] + 2 * key_len - late_skip, saved_salt->passverify, 2)) to use my +++ b/src/zip_fmt_plug.c
@@ -141,7 +141,7 @@ static int crypt_all(int *pcount, struct db_salt *salt)
pbkdf2_sha1_sse((const unsigned char **)pin, lens, saved_salt->salt, SALT_LENGTH(saved_salt->v.mode),
KEYING_ITERATIONS, pout, BLK_SZ, early_skip);
for (i = 0; i < MIN_KEYS_PER_CRYPT; ++i)
- if (!memcmp(pwd_ver[i] + 2 * key_len - late_skip, saved_salt->passverify, 2))
+ if (!memcmp(pwd_ver[i] + 0 + 2 * key_len - late_skip, saved_salt->passverify, 2))
something_hit = hits[i] = 1;
if (something_hit) {
for (i = 0; i < MIN_KEYS_PER_CRYPT; ++i) |
I think we should add the |
So ubsan is not good at analyzing and doing "math" on this kind of "complex" line of code. Probably, parentheses would also have fixed the issue. This is odd. |
Moreover, this was the real issue! I'm embarrassed I didn't realize at first. So the only UbSan bug here is that |
The text was updated successfully, but these errors were encountered: