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

UBSAN error in gflags (runtime error: applying non-zero offset), reported by Clang 12 #10046

Closed
mbautin opened this issue Sep 18, 2021 · 2 comments
Assignees

Comments

@mbautin
Copy link
Contributor

mbautin commented Sep 18, 2021

Running tests under AlmaLinux 8 / ASAN / Clang 12.

ts3|pid5159|:27888|http://127.228.44.111:32561 /home/yugabyteci/code/yugabyte-db-thirdparty/src/gflags-2.1.2/src/gflags.cc:1252:49: runtime error: applying non-zero offset 1 to null pointer
ts3|pid5159|:27888|http://127.228.44.111:32561     #0 0x7f10c21ef359 in google::(anonymous namespace)::CommandLineFlagParser::ProcessOptionsFromStringLocked(std::__1::basic_string<char, std::__1:
:char_traits<char>, std::__1::allocator<char> > const&, google::FlagSettingMode) (/home/yugabyteci/code/yugabyte-db-updated-libunwind/build/asan-clang12-dynamic-ninja/bin/../../../../yugabyte-db-
thirdparty/installed/asan/lib/libgflags.so.2+0x4d359)
ts3|pid5159|:27888|http://127.228.44.111:32561     #1 0x7f10c21f88e7 in google::(anonymous namespace)::CommandLineFlagParser::ProcessFlagfileLocked(std::__1::basic_string<char, std::__1::char_tra
its<char>, std::__1::allocator<char> > const&, google::FlagSettingMode) (/home/yugabyteci/code/yugabyte-db-updated-libunwind/build/asan-clang12-dynamic-ninja/bin/../../../../yugabyte-db-thirdpart
y/installed/asan/lib/libgflags.so.2+0x568e7)
ts3|pid5159|:27888|http://127.228.44.111:32561     #2 0x7f10c21ed0e1 in google::(anonymous namespace)::CommandLineFlagParser::ProcessSingleOptionLocked(google::(anonymous namespace)::CommandLineF
lag*, char const*, google::FlagSettingMode) (/home/yugabyteci/code/yugabyte-db-updated-libunwind/build/asan-clang12-dynamic-ninja/bin/../../../../yugabyte-db-thirdparty/installed/asan/lib/libgfla
gs.so.2+0x4b0e1)
ts3|pid5159|:27888|http://127.228.44.111:32561     #3 0x7f10c21f3738 in google::ParseCommandLineFlagsInternal(int*, char***, bool, bool) (/home/yugabyteci/code/yugabyte-db-updated-libunwind/build
/asan-clang12-dynamic-ninja/bin/../../../../yugabyte-db-thirdparty/installed/asan/lib/libgflags.so.2+0x51738)
ts3|pid5159|:27888|http://127.228.44.111:32561     #4 0x7f10c8067186 in yb::ParseCommandLineFlags(int*, char***, bool) /home/yugabyteci/code/yugabyte-db-updated-libunwind/build/asan-clang12-dynam
ic-ninja/../../src/yb/util/flags.cc:281:13
ts3|pid5159|:27888|http://127.228.44.111:32561     #5 0x428b26 in yb::tserver::(anonymous namespace)::TabletServerMain(int, char**) /home/yugabyteci/code/yugabyte-db-updated-libunwind/build/asan-
clang12-dynamic-ninja/../../src/yb/tserver/tablet_server_main.cc:171:3
ts3|pid5159|:27888|http://127.228.44.111:32561     #6 0x4282c0 in main /home/yugabyteci/code/yugabyte-db-updated-libunwind/build/asan-clang12-dynamic-ninja/../../src/yb/tserver/tablet_server_main
.cc:330:10
ts3|pid5159|:27888|http://127.228.44.111:32561     #7 0x7f10bd9e2492 in __libc_start_main (/lib64/libc.so.6+0x23492)
ts3|pid5159|:27888|http://127.228.44.111:32561     #8 0x4281ad in _start (/home/yugabyteci/code/yugabyte-db-updated-libunwind/build/asan-clang12-dynamic-ninja/bin/yb-tserver+0x4281ad)
ts3|pid5159|:27888|http://127.228.44.111:32561
@mbautin mbautin self-assigned this Sep 18, 2021
@mbautin mbautin changed the title UBSAN error in gflags, reported by Clang 12 UBSAN error in gflags (runtime error: applying non-zero offset), reported by Clang 12 Sep 18, 2021
@mbautin
Copy link
Contributor Author

mbautin commented Sep 18, 2021

The function in gflags.cc that has the issue:

   1243 string CommandLineFlagParser::ProcessOptionsFromStringLocked(
   1244     const string& contentdata, FlagSettingMode set_mode) {
   1245   string retval;
   1246   const char* flagfile_contents = contentdata.c_str();
   1247   bool flags_are_relevant = true;   // set to false when filenames don't match
   1248   bool in_filename_section = false;
   1249
   1250   const char* line_end = flagfile_contents;
   1251   // We read this file a line at a time.
   1252   for (; line_end; flagfile_contents = line_end + 1) {. // <-- UBSAN ERROR HAPPENS HERE
   1253     while (*flagfile_contents && isspace(*flagfile_contents))
   1254       ++flagfile_contents;
   1255     line_end = strchr(flagfile_contents, '\n');
   1256     size_t len = line_end ? line_end - flagfile_contents
   1257                           : strlen(flagfile_contents);
   1258     string line(flagfile_contents, len);
   1259
   1260     // Each line can be one of four things:
   1261     // 1) A comment line -- we skip it
   1262     // 2) An empty line -- we skip it
   1263     // 3) A list of filenames -- starts a new filenames+flags section
   1264     // 4) A --flag=value line -- apply if previous filenames match
   1265     if (line.empty() || line[0] == '#') {
   1266       // comment or empty line; just ignore
   1267
   1268     } else if (line[0] == '-') {    // flag
   1269       in_filename_section = false;  // instead, it was a flag-line
   1270       if (!flags_are_relevant)      // skip this flag; applies to someone else
   1271         continue;
   1272
   1273       const char* name_and_val = line.c_str() + 1;    // skip the leading -
   1274       if (*name_and_val == '-')
   1275         name_and_val++;                               // skip second - too
   1276       string key;
   1277       const char* value;
   1278       string error_message;
   1279       CommandLineFlag* flag = registry_->SplitArgumentLocked(name_and_val,
   1280                                                              &key, &value,
   1281                                                              &error_message);
   1282       // By API, errors parsing flagfile lines are silently ignored.
   1283       if (flag == NULL) {
   1284         // "WARNING: flagname '" + key + "' not found\n"
   1285       } else if (value == NULL) {
   1286         // "WARNING: flagname '" + key + "' missing a value\n"
   1287       } else {
   1288         retval += ProcessSingleOptionLocked(flag, value, set_mode);
   1289       }
   1290

@mbautin
Copy link
Contributor Author

mbautin commented Oct 5, 2021

The flagfile_contents = line_end + 1 part has undefined behavior if line_end is nullptr, but it does not matter because the loop condition (line_end) will cause the loop to exit in that case. We can suppress this warning in ubsan-suppressions.txt.

mbautin added a commit to mbautin/yugabyte-db that referenced this issue Oct 30, 2021
…ugabyte#10230] [yugabyte#10251] [yugabyte#10295] Enable Clang 12 ASAN build on AlmaLinux 8 and fix relevant bugs

Summary:
Enabling the Clang 12 ASAN build on AlmaLinux 8 and fixing these bugs from that build:
- yugabyte#7092 - pick up an updated version of LLVM where libunwind has been patched to work around crashing with an unknown x86_64 register error.
- yugabyte#10046 - suppress harmless undefined behavior in gflags.cc.
- yugabyte#10222 - increase the timeout for waiting for tablet server registration in mini cluster.
- yugabyte#10224 - when looping to wait for leader/follower of a tablet in TestUpdateLagMetrics, reset tablet server pointers to nullptr at every iteration.
- yugabyte#10230, yugabyte#10251 - use posix_spawn on Linux to create subprocesses to avoid getting stuck in the child process when allocating memory when setting environment variables because some memory allocation lock is already held by the parent process. On macOS, we still use fork+exec because posix_spawnp throws errors related to closing file descriptors. Making sure all files in our code are opened with FD_CLOEXEC is out of scope of this revision, and will be addressed by yugabyte#10321.
- yugabyte#10295 - fix undefined behavior (adding to a null pointer) in a Postgres sorting function.

Also removing Clang 7 ASAN build on CentOS 7 from Jenkins (we don't need two ASAN builds).

Test Plan: Jenkins

Reviewers: bogdan, steve.varnau, sergei

Reviewed By: sergei

Subscribers: ybase

Differential Revision: https://phabricator.dev.yugabyte.com/D13301
mbautin added a commit that referenced this issue Oct 30, 2021
…Clang 12 ASAN build on AlmaLinux 8 and fix relevant bugs

Summary:
Updating third-party dependencies from 44ba3719652858e1f7816df87b25101e09527c88 to 2d282c38cfcbc10af7bdc1c86bf1e8af88f36efd (updating the LLVM toolchain with a bug fix, and also picking up the program_options Boost library addition by @mikhpolitov, commit yugabyte/yugabyte-db-thirdparty@2d282c3).

yugabyte-db-thirdparty diff link:
https://github.com/yugabyte/yugabyte-db-thirdparty/compare/44ba3719652858e1f7816df87b25101e09527c88..2d282c38cfcbc10af7bdc1c86bf1e8af88f36efd

Enabling the Clang 12 ASAN build on AlmaLinux 8 and fixing these bugs from that build:
- #7092 - pick up an updated version of LLVM where libunwind has been patched to work around crashing with an unknown x86_64 register error.
- #10046 - suppress harmless undefined behavior in gflags.cc.
- #10222 - increase the timeout for waiting for tablet server registration in mini cluster.
- #10224 - when looping to wait for leader/follower of a tablet in TestUpdateLagMetrics, reset tablet server pointers to nullptr at every iteration.
- #10230, #10251 - use posix_spawn on Linux to create subprocesses to avoid getting stuck in the child process when allocating memory when setting environment variables because some memory allocation lock is already held by the parent process. On macOS, we still use fork+exec because posix_spawnp throws errors related to closing file descriptors. Making sure all files in our code are opened with FD_CLOEXEC is out of scope of this revision, and will be addressed by #10321.
- #10295 - fix undefined behavior (adding to a null pointer) in a Postgres sorting function.
- Fix HostPort& field type in ysql_upgrade.h (make it HostPort). Otherwise there is an ASAN issue accessing a deallocated stack value. (No separate GitHub issue for this bug.)

Also consolidating Status generation from a C standard library error number in errno.h and errno.c, and adding a few utility macros for convenient invocation of C functions that either return an errno as the return value, or return zero vs. non zero depending on whether there is an error, and set errno as a side effect.

As part of this diff, we are removing Clang 7 ASAN build on CentOS 7 from Jenkins (we don't need two different ASAN builds).

Test Plan: Jenkins

Reviewers: bogdan, steve.varnau, sergei

Reviewed By: sergei

Subscribers: ybase

Differential Revision: https://phabricator.dev.yugabyte.com/D13301
@mbautin mbautin closed this as completed May 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant