Skip to content
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

Making base64 cctest deterministic makes it fail #13636

Closed
addaleax opened this issue Jun 12, 2017 · 4 comments · Fixed by #13660
Closed

Making base64 cctest deterministic makes it fail #13636

addaleax opened this issue Jun 12, 2017 · 4 comments · Fixed by #13660
Labels
buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. test Issues and PRs related to the tests.

Comments

@addaleax
Copy link
Member

The following patch makes the base64 cctest fail for me:

diff --git a/test/cctest/test_base64.cc b/test/cctest/test_base64.cc
index fbdb969b4cbe..ecb61aa84cea 100644
--- a/test/cctest/test_base64.cc
+++ b/test/cctest/test_base64.cc
@@ -12,7 +12,7 @@ TEST(Base64Test, Encode) {
   auto test = [](const char* string, const char* base64_string) {
     const size_t len = strlen(base64_string);
     char* const buffer = new char[len + 1];
-    buffer[len] = 0;
+    memset(buffer, 0, len+1);
     base64_encode(string, strlen(string), buffer, len);
     EXPECT_STREQ(base64_string, buffer);
     delete[] buffer;
@@ -48,7 +48,7 @@ TEST(Base64Test, Decode) {
   auto test = [](const char* base64_string, const char* string) {
     const size_t len = strlen(string);
     char* const buffer = new char[len + 1];
-    buffer[len] = 0;
+    memset(buffer, 0, len+1);
     base64_decode(buffer, len, base64_string, strlen(base64_string));
     EXPECT_STREQ(string, buffer);
     delete[] buffer;

That shouldn’t be happening, because buffer might be 0-initialized anyway. Without this patch, valgrind complains that we use uninitialized parts of buffer (rightfully so, I assume).

I don’t have the time to look into this right now, but I assume either the test is just asserting the wrong values, or something’s off with our base64 decoder implementation.

/cc @aqrln @seishun

@addaleax addaleax added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. test Issues and PRs related to the tests. labels Jun 12, 2017
@seishun
Copy link
Contributor

seishun commented Jun 12, 2017

Not familiar with cctest. How do I run (or preferably, debug) them?

@addaleax
Copy link
Member Author

@seishun You can run them with ./out/Release/cctest or ./out/Debug/cctest, depending on whether you have a debug build or not; for debugging, I’m not really sure how to answer that question beyond “use your favourite C++ debugger” or “add fprintf(stderr, …); in lots of places”…

@seishun
Copy link
Contributor

seishun commented Jun 13, 2017

Great, so you can just debug cctest as usual. Things are more complicated with libuv tests due to subprocesses.

That shouldn’t be happening, because buffer might be 0-initialized anyway.

In theory, yes. In practice, buffer is pretty much guaranteed to contain the result of the previous test. Since base64_decode doesn't null-terminate the result string, then the test will incorrectly succeed if the new result is just shorter. By the way, I can reproduce the test failure in a debug build even without your patch.

I'll try to look into what's going on in base64_decode, but I'm sure someone can figure it out faster than me.

@addaleax
Copy link
Member Author

Great, so you can just debug cctest as usual. Things are more complicated with libuv tests due to subprocesses.

You could try to filter using cctest --gtest_filter='*Decode*' to only get the failing test, maybe that helps?

In practice, buffer is pretty much guaranteed to contain the result of the previous test. If base64_decode somehow fails to null-terminate the result string, then the test will incorrectly succeed.

Makes sense… :/

addaleax pushed a commit that referenced this issue Jun 17, 2017
`max_i` should also include the characters that were just read by
`base64_decode_group_slow()`.

PR-URL: #13660
Fixes: #13636
Fixes: #13657
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit that referenced this issue Jun 21, 2017
`max_i` should also include the characters that were just read by
`base64_decode_group_slow()`.

PR-URL: #13660
Fixes: #13636
Fixes: #13657
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants