-
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
Audit rules engine optimizations #3517
Comments
@solardiz [List.Rules:AppendSeason]
- <* Az"[Ss$][uU][mM][mM][eE3][rR]"
- <* Az"[Ww][iI|][nN][tT+][eE3][rR]"
- <* Az"[Ff][aA][lL][lL]"
- <* Az"[Ss][pP][rR][iI][nN][gG]"
- <* Az"[Aa][uU][tT][uU][mM][nN]"
+ a6 Az"[Ss$][uU][mM][mM][eE3][rR]"
+ a6 Az"[Ww][iI|][nN][tT+][eE3][rR]"
+ a6 Az"[Ff][aA][lL][lL]"
+ a6 Az"[Ss][pP][rR][iI][nN][gG]"
+ a6 Az"[Aa][uU][tT][uU][mM][nN]" I'm not sure this is right. Does the OK, the problem you mention is for real, but it's not new with What we could do in Jumbo is this: diff --git a/src/rules.c b/src/rules.c
index 705c841ae..c41d1c24b 100644
--- a/src/rules.c
+++ b/src/rules.c
@@ -619,6 +619,8 @@ void rules_init(struct db_main *db, int max_length)
min_length = options.eff_minlength;
skip_length = options.force_maxlength;
+ if (db->format->params.flags & FMT_TRUNC && !skip_length)
+ max_length = PLAINTEXT_BUFFER_SIZE - 3;
if (max_length == rules_max_length)
return; |
(there's some more to it than the diff above, but that's the gist of it) |
@magnumripper I think you picked a wrong example. I intentionally chose "Summer" and not the numbers, etc. For the numbers, etc., we assume we also have rules that test shorter ones, so testing truncated numbers would be redundant. This is not the case for words like "Summer". I don't understand the patch you propose, but FWIW it needs braces around |
variable(s) to 125 (+/-1) instead of format's reported length. See #3517.
OK, I thought you placed your comment near the rules you discussed. I edited my post. Anyway, that makes more sense. You mean we should only reject if we can't append even one letter of it. |
For the Summer case, the only sane way to get it the way you want is to revert that change. Personally I'd rather have it with I should probably just ditch #3522. What it does is setting rules_max_length as well as I think we need to discuss exactly how and when we should take FMT_TRUNC in consideration. |
Here's current behavior. @solardiz is there anything we want to change here? If not, I will add some of these details to the docs, and then I can go on with the OP possibly only reverting some rule changes.
The above refer to format's reported length, unless overridden with
Same here, as stated in the text. And we're ignoring
Same here, regardless of FMT_TRUNC. And Last of all, in Note that the Also note that you can make eg. DEScrypt lose it's |
I'm really not sure I want this. Perhaps that behavior should be dropped from |
...and perhaps we don't want that even for Perhaps we should have separate commands and flags doing the same as these "unless more rules are coming". |
I'm afraid this got beyond the complexity level that I currently have time to fully consider. :-( So I'll add just one comment: in the "Summer" case, we might or might not want to append just the letter "S" (if that's the only one that fits) depending on whether we also have a rule that tries appending all uppercase letters (or all characters) or not. Ditto for 2 or maybe even 3 characters depending on whether we have such multi-character append rules. It's that tricky. OTOH, with such multi-character appends present, trying to append the word unconditionally may only result in relatively negligible overhead (like a few extra rules appending specific words on top of, say, 95^2 or 26^3 rules appending character combinations). |
@magnumripper Probably yes, assuming you have no time to properly test it and fix whatever needs fixing. |
@magnumripper The changes you made per this issue that are still not reverted remain problematic. Specifically, your uses of the See e.g. the "Summer" example above, which I've just tested against You also use a lot of That said, some of the uses of
is just right as long as we also have single-letter appends. There are many other such examples. However, this is wrong for the truncation case (it's right for the
I still don't nor expect to have time to dive into this, but I do feel the need to point out that this is still problematic. |
I kind of wish I had never implemented these but reverting all of it now would cause compatibility issues.
A combination of both sounds like a plan. I should at least look into changing the behavior asap - hopefully a trivial fix. |
See #3468, 15163f4#commitcomment-31692841 and 7955900#commitcomment-31692820
aN
behaves correctly depending on theFMT_TRUNC
flag.-c
somewhere where it should be-[:c]
(followed by\p1[lu]
).The text was updated successfully, but these errors were encountered: