{"action":"created","issue":{"url":"https://api.github.com/repos/okdshin/PicoSHA2/issues/2","repository_url":"https://api.github.com/repos/okdshin/PicoSHA2","labels_url":"https://api.github.com/repos/okdshin/PicoSHA2/issues/2/labels{/name}","comments_url":"https://api.github.com/repos/okdshin/PicoSHA2/issues/2/comments","events_url":"https://api.github.com/repos/okdshin/PicoSHA2/issues/2/events","html_url":"https://github.com/okdshin/PicoSHA2/pull/2","id":187470157,"number":2,"title":"allow carry > 1 in add_to_data_length().","user":{"login":"forrestcavalier","id":3105258,"avatar_url":"https://avatars.githubusercontent.com/u/3105258?v=3","gravatar_id":"","url":"https://api.github.com/users/forrestcavalier","html_url":"https://github.com/forrestcavalier","followers_url":"https://api.github.com/users/forrestcavalier/followers","following_url":"https://api.github.com/users/forrestcavalier/following{/other_user}","gists_url":"https://api.github.com/users/forrestcavalier/gists{/gist_id}","starred_url":"https://api.github.com/users/forrestcavalier/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/forrestcavalier/subscriptions","organizations_url":"https://api.github.com/users/forrestcavalier/orgs","repos_url":"https://api.github.com/users/forrestcavalier/repos","events_url":"https://api.github.com/users/forrestcavalier/events{/privacy}","received_events_url":"https://api.github.com/users/forrestcavalier/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2016-11-05T01:57:53Z","updated_at":"2016-11-07T18:49:36Z","closed_at":"2016-11-06T09:34:07Z","pull_request":{"url":"https://api.github.com/repos/okdshin/PicoSHA2/pulls/2","html_url":"https://github.com/okdshin/PicoSHA2/pull/2","diff_url":"https://github.com/okdshin/PicoSHA2/pull/2.diff","patch_url":"https://github.com/okdshin/PicoSHA2/pull/2.patch"},"body":"Maintain 16 bit segments. Unlikely to cause error unless doing multi-GB lengths in chunks >= 65536*2.\r\n(Noticed in code review when rewriting to straight C.)"},"comment":{"url":"https://api.github.com/repos/okdshin/PicoSHA2/issues/comments/258925635","html_url":"https://github.com/okdshin/PicoSHA2/pull/2#issuecomment-258925635","issue_url":"https://api.github.com/repos/okdshin/PicoSHA2/issues/2","id":258925635,"user":{"login":"forrestcavalier","id":3105258,"avatar_url":"https://avatars.githubusercontent.com/u/3105258?v=3","gravatar_id":"","url":"https://api.github.com/users/forrestcavalier","html_url":"https://github.com/forrestcavalier","followers_url":"https://api.github.com/users/forrestcavalier/followers","following_url":"https://api.github.com/users/forrestcavalier/following{/other_user}","gists_url":"https://api.github.com/users/forrestcavalier/gists{/gist_id}","starred_url":"https://api.github.com/users/forrestcavalier/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/forrestcavalier/subscriptions","organizations_url":"https://api.github.com/users/forrestcavalier/orgs","repos_url":"https://api.github.com/users/forrestcavalier/repos","events_url":"https://api.github.com/users/forrestcavalier/events{/privacy}","received_events_url":"https://api.github.com/users/forrestcavalier/received_events","type":"User","site_admin":false},"created_at":"2016-11-07T18:49:36Z","updated_at":"2016-11-07T18:49:36Z","body":"You are very welcome! I was very happy to find your compact SHA256 under the MIT \nlicense. Before I found your version, I worked with the LibreSSL/OpenSSL \nversion, which relies heavily on macro expansion and has clumsy licensing (with \n3 copyright holders claimed!)\n\nFor a new test case for picoSHA2, I don't know what to suggest. I did not find \nthe problem by testing.  I reasoned about the data structure that held the \nlength: 4 segments of 16 bits each.\n\nIf PICOSHA2 had code which checked that each segment is limited to 16 bits, or \nsubsystem tested just the add operation without the SHA rounds, the bug could be \ndetected by running a block above 2* 65536 and see that check failed.\n\nWhen I saw the behavior not reducing carries above 1, I was surprised the SHA256 \noutput was still correct! This is explained because I was using small test cases \n(1 million characters) and the PicoSHA2 method to convert to bitlength to \nfinalize the last block: the segment which was above 16 bits was handled \ncorrectly anyway.  If the code had used a different method to multiply by 8 that \nignored the upper 16 bits of each segment, there would have been an error for a \nsingle block with length above 2*65536 bytes.\n\nTest cases for this bug must be very large, for example more than 65536 blocks \neach of length above 2 * 65536.  With that kind of processing, the carry will \noverflow out of the 32-bit segment and is lost: it cannot be recovered at the \nmultiply by 8.\n\nTest cases that are long enough to trigger the bug are not practical. I don't \nthink you want it in your test set.  Maybe you can add subsystem testing for the \n16-bit segmented additions?  (It is hard for me to suggest something that is \ngood: You have nice clean code, and adding extra tests and methods to access \nprivate members just for debugging seems unnecessary.)\n\nI created a test which repeats a call to .process with a block of 250000 \nrepetitions of the character 'a', basically this:\n\nstd::string src_str(250000,'a');\nfor(int i = 0; i < 65536;i++) {\n     hasher.process(src_str.begin(), src_str.end());\n}\nThat is about a 16GB input string, and it takes many minutes to run.\n\nWithout the carry bug fix,  the result is:\n50a63e21a77e2acd038542a7f774b90146a70a80172330d9f9a01da0bbf1c8f0\n\nWith the carry fix, the result is:\n00e94ed5771326935293ebbb9831d811bd1d8dd5b4690def8e2bc87c56f789c6\n\nThis difference shows the bug fix is important in these kinds of uses.\n\nThank you for accepting the pull request so quickly!\n\n\n\n\nOn 11/6/2016 4:34 AM, s.okada wrote:\n>\n> Thank you very much!\n> I was not aware of it.\n> If you wouldn't mind, please add a code snippet to test it.\n>\n> —\n> You are receiving this because you authored the thread.\n> Reply to this email directly, view it on GitHub \n> <https://github.com/okdshin/PicoSHA2/pull/2#issuecomment-258669659>, or mute \n> the thread \n> <https://github.com/notifications/unsubscribe-auth/AC9h6lQqNyePv9gEF0LV_5nTl3rp8eY1ks5q7Z8RgaJpZM4KqIhG>.\n>\n\n"}}
{"actor":{"display_login":"forrestcavalier"}}