[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <55A281D2.8070203@openwall.com>
Date: Sun, 12 Jul 2015 18:03:46 +0300
From: Alexander Cherepanov <ch3root@...nwall.com>
To: john-dev@...ts.openwall.com
Subject: Re: more robustness
On 2015-06-27 16:31, Kai Zhao wrote:
>> >1. Add more fuzzing methods
>> >2. Support formats whose ciphertext is very long such as LUKS
>> >3. Add the split() after valid()
> The attachments are two patches which finished the 3 works of the
> last patch. So I think --fuzz is finished. Am I right? Do you have any
> advice?
Thanks for posting this. Some comments are inline.
> +void fuzz_init_dictionary()
> +{
> + FILE *file;
> + char line_buf[LINE_BUFFER_SIZE], *p;
> + struct FuzzDic *last_fd, *pfd;
> +
> + if (!options.fuzz_dic)
> + return;
> +
> + if (!(file = fopen(options.fuzz_dic, "r")))
> + pexit("fopen: %s", options.fuzz_dic);
> +
> + rfd = mem_alloc(sizeof(struct FuzzDic));
> + rfd->next = NULL;
> + last_fd = rfd;
> + while (fgets(line_buf, sizeof(line_buf), file)) {
> +
> + p = line_buf;
> + while (*p) {
> + if ('\n' == *p) {
> + *p = 0;
> + break;
> + }
> + p++;
> + }
> +
> + pfd = mem_alloc(sizeof(struct FuzzDic));
> + pfd->next = NULL;
> + pfd->value = strdup(line_buf);
> + last_fd->next = pfd;
> + last_fd = pfd;
> + }
> +
> + if (ferror(file)) pexit("fgets");
> +
> + if (fclose(file)) pexit("fclose");
> +}
If you are reading a whole file into memory (and I think we are not
concerned with reading from pipes) then it's easier to get its size and
allocate its full size in one go and read it in as one chunk. Then got
through it replacing \n with 0 and saving addresses in a list. Or
eliminate the list entirely.
> +static char * get_next_fuzz_case(char *label, char *ciphertext)
> +{
> + static int is_replace_finish = 0; // is_replace_finish = 1 if all the replaced cases have been generated
> + static int is_swap_finish = 0; // is_swap_finish = 1 if all the swaped cases have been generated
> + static int is_append_finish = 0; // is_append_finish = 1 if all the appended cases have been generated
> + static int is_chgcase_finish = 0; // is_chgcase_finish = 1 if all the change cases have been generated
> + static int is_insertdic_finish = 0; // is_insertdic_finish = 1 if all the insert dictionary cases have been generated
> + static char *last_label = NULL, *last_ciphertext = NULL;
> +
> + memcpy(fuzz_hash, ciphertext, strlen(ciphertext));
Why don't you use strcpy and other str* functions here and in similar cases?
> + fuzz_hash[strlen(ciphertext)] = 0;
> +
> + if (!last_label)
> + last_label = label;
> +
> + if (!last_ciphertext)
> + last_ciphertext = ciphertext;
> +
> + if (strcmp(label, last_label) != 0 || strcmp(ciphertext, last_ciphertext) != 0) {
> + is_replace_finish = 0;
> + is_swap_finish = 0;
> + is_append_finish = 0;
> + is_chgcase_finish = 0;
> + is_insertdic_finish = 0;
> + last_label = label;
> + last_ciphertext = ciphertext;
> + }
> +
> + if (!is_replace_finish)
> + if (NULL != replace_each_chars(ciphertext, &is_replace_finish))
In many cases you put a constant in a comparison on the left. It was
useful to some degree some time ago (to catch = instead of == in
conditions). But it's quite unintuitive and current compilers warn about
most improper cases (if you don't have parenthesis around the comparison).
IMHO it's better to move all constants to the right.
And my personal preference is to drop checks "!= NULL" completely. But
this is something for a debate.
--
Alexander Cherepanov
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
