Review all formats' prepare functions

[magnumripper]

#4430 (comment)

In short:

• prepare need to be lean and early reject if possible
• If prepare knows the line wont pass valid, it should return NULL. This tells loader not to bother.

[magnumripper]

Most or all prepare functions I see never ever return NULL, they return fields[1] even when they know it will not pass valid.

I also see several things like this (this is NETLMv2_fmt_plug.c):

	tmp = (char *) mem_alloc(9 + strlen(identity) + 1 + strlen(srv_challenge) + 1 + strlen(nethashv2) + 1 + strlen(cli_challenge) + 1);
	sprintf(tmp, "%s%s$%s$%s$%s", FORMAT_TAG, identity, srv_challenge, nethashv2, cli_challenge);
	MEM_FREE(identity);

	if (valid(tmp, self)) {
		char *cp = str_alloc_copy(tmp);
		MEM_FREE(tmp);
		return cp;
	}
	MEM_FREE(tmp);

Why on earth would we copy the string, free the original one and return the copy? Just return tmp and be done with it. This is after passing valid so not that important, but it's stupid anyway.

[magnumripper]

BTW I really dislike prepare calling valid at all. I can see it's sometimes called for (like in LMv2 above, after trying a construct - if it doesn't pass we'd like to free it) but for an early rejection a simple strncmp feels better. In the end it may not matter much (or at all) but it gives me the creeps.

Speaking of alloc/free... Shouldn't we better use static string buffers instead? In the LMv2 case above we could then just return the construct, and loader will try it with valid.

[magnumripper]

• If prepare knows the line wont pass valid, it should return NULL. This tells loader not to bother.

I see now not all parts of core (but most) support a NULL return from prepare(). I suggest we document that as being not only valid but the right thing to do whenever prepare had enough peek of the input to be able to tell already.

These seem to be the only missing pieces:

diff --git a/src/cracker.c b/src/cracker.c
index b1826d85e..16b7d23c9 100644
--- a/src/cracker.c
+++ b/src/cracker.c
@@ -652,7 +652,7 @@ int crk_reload_pot(void)
 		fields[0] = "";
 		fields[1] = ciphertext;
 		ciphertext = crk_methods.prepare(fields, crk_db->format);
-		if (ldr_trunc_valid(ciphertext, crk_db->format)) {
+		if (ciphertext && ldr_trunc_valid(ciphertext, crk_db->format)) {
 			ciphertext = crk_methods.split(ciphertext, 0,
 			                               crk_db->format);
 			if (crk_remove_pot_entry(ciphertext))
diff --git a/src/formats.c b/src/formats.c
index 395f38499..e75c6db7c 100644
--- a/src/formats.c
+++ b/src/formats.c
@@ -1535,7 +1535,7 @@ static void test_fmt_split_unifies_case(struct fmt_main *format, char *ciphertex
 	flds[0] = fake_user;
 	flds[1] = cipher_copy;
 	ret = format->methods.prepare(flds, format);
-	if (format->methods.valid(ret, format)) {
+	if (ret && format->methods.valid(ret, format)) {
 		char *cp;
 		int do_test=0;
 		ret = format->methods.split(ret, 0, format);
@@ -1833,7 +1833,7 @@ static void test_fmt_split_unifies_case_4(struct fmt_main *format, char *ciphert
 	flds[0] = fake_user;
 	flds[1] = cipher_copy;
 	ret = format->methods.prepare(flds, format);
-	if (format->methods.valid(ret, format)) {
+	if (ret && format->methods.valid(ret, format)) {
 		char *cp;
 		ret = format->methods.split(ret, 0, format);
 		ret_copy = strdup(ret);
diff --git a/src/loader.c b/src/loader.c
index 10e90918d..6054dc6f5 100644
--- a/src/loader.c
+++ b/src/loader.c
@@ -710,7 +710,7 @@ find_format:
 #endif
 #endif
 			prepared = alt->methods.prepare(fields, alt);
-			if (alt->methods.valid(prepared, alt)) {
+			if (prepared && alt->methods.valid(prepared, alt)) {
 				alt->params.flags |= FMT_WARNED;
 				if (john_main_process)
 				fprintf(stderr,