Commit 3d8ec487 authored by Victor Costan's avatar Victor Costan Committed by Commit Bot

sqlite: Prefix SQLite API methods with chrome_.

In component builds, SQLite's API methods are exported from the
chromium_sqlite component, which means they are visible to the dynamic
library loader. This opens up the following possibilities:

1) A system library calls into our SQLite instead of calling into the
   system's SQLite library which it was built against. The patches in
   our SQLite version lead to different behavior from the system's
   SQLite, which can cause subtle failures. This happens if the dynamic
   library loader resolves the system library's symbol imports with our
   SQLite's exported symbols.
2) A system library loads the system SQLite, and we end up calling into
   it, instead of calling into our version of SQLite. This happens if
   the dynamic library loader resolves our symbol imports with the
   system's SQLite library.

Both possibilities above lead to the possibility that the component
build will behave differently from the release build, in subtle and
potentially non-deterministic ways. This is not a purely academic
concern. https://crbug.com/807487 happened because we use NSS on Linux,
and NSS invokes SQLite via a complex plugin system. On non-component builds,
NSS (a system library) loads and uses the system version of SQLite. On
component builds, NSS ends up using our SQLite.

This CL fixes the problem by adding a chrome_ prefix to all the symbols
exported from SQLite3. In C++ libraries, namespaces can make prefixing
easy. Unfortunately, SQLite is a C library, so the prefixing is fairly
heavy-handed. A high-level overview of the approach follows:

* An extract_sqlite_api Python script reads SQLite's header, extracts
  the names of all exported symbols, and writes a header file consisting
  of renaming preprocessor macros, e.g.
      #define sqlite3_init chrome_sqlite3_init
  David Benjamin <davidben@chromium.org> designed the approach and wrote
  the original version of the script.
* The script that we use to generate SQLite's amalgamation now also
  invokes the extract_sqlite_api script described above, and saves the
  output to amalgamation/rename_exports.h.
* The SQLite component exposes an sqlite3.h header that must be used by
  all SQLite3 users in Chromium. This header now #includes
  rename_exports.h (containing the renaming preprocessor macros) before
  #including amalgamation/sqlite3.h.
* sqlite3.c (the main output of the amalgamation process) does not
  #include "sqlite3.h". However, in order to facilitate autoconf builds,
  it does #include a "config.h", if a certain preprocessor define
  exists. We abuse that define to have sqlite.c always load config.h,
  and have config.h load our rename_exports.h.

This CL also adds a PRESUBMIT.py that runs unit tests for the
extract_sqlite_api Python script, which ensures that the script will not
break accidentally. Both the unit tests and the PRESUBIMT script are
inspired from //tools/vim.

Bug: 807093, 807487
Change-Id: If3868ba119ffd4ccbb06d1a6fcd4cc2ecd9ef2ae
Reviewed-on: https://chromium-review.googlesource.com/898549Reviewed-by: default avatarChris Mumford <cmumford@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534843}
parent 5157f928
...@@ -75,13 +75,14 @@ config("chromium_sqlite3_compile_options") { ...@@ -75,13 +75,14 @@ config("chromium_sqlite3_compile_options") {
# syntax working but execution failing. Review: # syntax working but execution failing. Review:
# src/src/parse.py # src/src/parse.py
# src/tool/mkkeywordhash.c # src/tool/mkkeywordhash.c
]
# Pull in config.h on Linux. This allows use of preprocessor macros which # Chromium does not use sqlite3_{enable_}load_extension().
# are not available to the build config. # Asides from giving us fairly minor code savings, this option disables code
if (is_linux) { # that breaks our method for renaming SQLite's exported symbols. Last,
defines += [ "_HAVE_SQLITE_CONFIG_H" ] # there's a tiny security benefit to knowing that WebSQL can't possibly
} # reach extension loading code.
"SQLITE_OMIT_LOAD_EXTENSION",
]
if (using_sanitizer) { if (using_sanitizer) {
# Limit max length of data blobs and queries for fuzzing builds by 128 MB. # Limit max length of data blobs and queries for fuzzing builds by 128 MB.
...@@ -138,7 +139,13 @@ if (!use_system_sqlite) { ...@@ -138,7 +139,13 @@ if (!use_system_sqlite) {
] ]
cflags = [] cflags = []
defines = [] defines = [
# The generated sqlite3.c does not include sqlite3.h, so we cannot easily
# inject the renaming macros in amalgamation/export_renames.h. However,
# if the macro below is defined, sqlite3.c will #include "config.h", which
# can be used to inject the macros.
"_HAVE_SQLITE_CONFIG_H",
]
if (is_component_build) { if (is_component_build) {
if (is_win) { if (is_win) {
...@@ -168,7 +175,10 @@ if (!use_system_sqlite) { ...@@ -168,7 +175,10 @@ if (!use_system_sqlite) {
] ]
} }
include_dirs = [ "amalgamation" ] include_dirs = [
".", # sqlite3.h here must override the one in amalgamation/.
"amalgamation",
]
configs -= [ "//build/config/compiler:chromium_code" ] configs -= [ "//build/config/compiler:chromium_code" ]
configs += [ configs += [
...@@ -222,11 +232,14 @@ if (!use_system_sqlite) { ...@@ -222,11 +232,14 @@ if (!use_system_sqlite) {
if (is_linux) { if (is_linux) {
executable("sqlite_shell") { executable("sqlite_shell") {
# So shell.c can find the correct sqlite3.h. include_dirs = [
include_dirs = [ "amalgamation" ] # shell.c contains an '#include "sqlite3.h", which we want to be
# resolved to //third_party/sqlite/shell.h.
".",
]
sources = [ sources = [
"amalgamation/shell.c", "amalgamation/shell/shell.c",
"src/src/shell_icu_linux.c", "src/src/shell_icu_linux.c",
# Include a dummy c++ file to force linking of libstdc++. # Include a dummy c++ file to force linking of libstdc++.
......
# Copyright 2018 The Chromium Authors. All rights reserved.
# Use of this source code is governed by a BSD-style license that can be
# found in the LICENSE file.
"""Presubmit tests for /third_party/sqlite.
Runs Python unit tests in /third_party/sqlite/scripts on upload.
"""
def CheckChangeOnUpload(input_api, output_api):
results = []
results += input_api.RunTests(
input_api.canned_checks.GetUnitTests(input_api, output_api, [
'scripts/extract_sqlite_api_unittest.py'
]))
return results
...@@ -42,7 +42,7 @@ The directory structure is as follows. Files common to all third_party projects ...@@ -42,7 +42,7 @@ The directory structure is as follows. Files common to all third_party projects
build, which merges all the code in one .c file and one .h build, which merges all the code in one .c file and one .h
file. See https://www.sqlite.org/amalgamation.html file. See https://www.sqlite.org/amalgamation.html
* amalgamation/config.h - Linux build configuration * amalgamation/config.h - Linux build configuration
* google_generate_amalgamation.sh - Script that generates the amalgamation * scripts/ - Scripts that generate the files in the amalgamation
* sqlite.h - The header used by the rest of Chromium to include SQLite. This * sqlite.h - The header used by the rest of Chromium to include SQLite. This
forwards to amalgamation/sqlite3.h forwards to amalgamation/sqlite3.h
* fuzz/ - Google OSS-Fuzz (ClusterFuzz) testing for Chromium's SQLite build * fuzz/ - Google OSS-Fuzz (ClusterFuzz) testing for Chromium's SQLite build
...@@ -54,7 +54,7 @@ The directory structure is as follows. Files common to all third_party projects ...@@ -54,7 +54,7 @@ The directory structure is as follows. Files common to all third_party projects
third_party/sqlite/src is the patched source from SQLite. This is used to third_party/sqlite/src is the patched source from SQLite. This is used to
generate the amalgamation, a concatenation of all of the files into a giant generate the amalgamation, a concatenation of all of the files into a giant
sqlite3.c. To prototype, edit in src/, then call sqlite3.c. To prototype, edit in src/, then call
./google_generate_amalgamation.sh ./scripts/generate_amalgamation.sh
to regenerate sqlite3.c. The code in src/ is much easier to edit, and the to regenerate sqlite3.c. The code in src/ is much easier to edit, and the
SQLite test framework can easily be run. During development it may be SQLite test framework can easily be run. During development it may be
convenient to modify BUILD.gn based on src/main.mk to just pull in the src/ convenient to modify BUILD.gn based on src/main.mk to just pull in the src/
...@@ -101,8 +101,8 @@ git add patches/*.patch ...@@ -101,8 +101,8 @@ git add patches/*.patch
git commit -m "Rebuild patches for sqlite_${BASE}" git commit -m "Rebuild patches for sqlite_${BASE}"
# Re-generate the amalgamation. # Re-generate the amalgamation.
./google_generate_amalgamation.sh ./scripts/generate_amalgamation.sh
git commit -m 'google_generate_amalgamation.sh' amalgamation/ git commit -m './scripts/generate_amalgamation.sh' amalgamation/
# At this point everything should build and work. # At this point everything should build and work.
# Do a squash upload. This should add your single patch to patches/, and apply # Do a squash upload. This should add your single patch to patches/, and apply
...@@ -204,16 +204,23 @@ git rebase sqlite-new-base ...@@ -204,16 +204,23 @@ git rebase sqlite-new-base
#### Finally, create the branch that we'll upload. #### Finally, create the branch that we'll upload.
git new-branch --upstream-current sqlite-new-cl git new-branch --upstream-current sqlite-new-cl
./google_generate_amalgamation.sh ./scripts/generate_amalgamation.sh
#### Validate the upgrade.
# The goal is to have a set of reasonably-independent CLs which can be # The goal is to have a set of reasonably-independent CLs which can be
# understood separately, so that future importers can sensibly determine how to # understood separately, so that future importers can sensibly determine how to
# handle conflicts. So use git-rebase and slipstream fixups back into their # handle conflicts. So use git-rebase and slipstream fixups back into their
# original CL until everything builds and works. # original CL until everything builds and works.
cd ../.. cd ../..
ninja -C out/Default ninja -C out/Default
# Check that extract_sqlite_api.py added chrome_ to all exported symbols.
# Only "_fini" and "_init" should be unprefixed.
nm -B out/Default/libchromium_sqlite3.so | cut -c 18- | sort | grep '^T'
out/Default/sql_unittests out/Default/sql_unittests
third_party/WebKit/Tools/Scripts/run-webkit-tests -t Default storage/websql/* third_party/WebKit/Tools/Scripts/run-webkit-tests -t Default storage/websql/*
cd third_party/sqlite cd third_party/sqlite
#### Create the review.
# Rebuild the patch set. # Rebuild the patch set.
git rm patches/* git rm patches/*
git format-patch --output-directory=patches --ignore-space-change \ git format-patch --output-directory=patches --ignore-space-change \
......
DO NOT EDIT FILES IN THIS DIRECTORY.
These files are automatically generated from the sqlite originals. If
you edit these files, your edits will be dropped in a future import of
the sqlite code.
See ../google_generate_amalgamation.sh for information on how these
files are built.
Scott Hess, April 6, 2011.
** DO NOT EDIT FILES IN THIS DIRECTORY. **
These files are automatically generated from the sqlite originals. If you
edit these files, your edits will be dropped in a future import of the sqlite
code.
See the contents of `../scripts` for information on how these files are
built.
\ No newline at end of file
/* On Windows and OSX, SQLite uses preprocessor macros to configure itself. On // Copyright 2018 The Chromium Authors. All rights reserved.
* Linux, it expects config.h from autoconf. autoconf generates config.h by // Use of this source code is governed by a BSD-style license that can be
* compiling a series of probe programs, and Chromium's build system has no // found in the LICENSE file.
* "configure" phase to put such generation in. This file is a workaround for
* this issue. #ifndef THIRD_PARTY_SQLITE_AMALGAMATION_CONFIG_H_
*/ #define THIRD_PARTY_SQLITE_AMALGAMATION_CONFIG_H_
/* TODO(shess): Expand this to OSX and Windows? */
/* TODO(shess): Consider config_linux.h, config_mac.h, config_win.h? */ // This file is included by sqlite3.c fairly early.
/* NOTE(shess): This file is included by sqlite3.c, be very careful about adding // We prefix chrome_ to SQLite's exported symbols, so that we don't clash with
* #include lines. // other SQLite libraries loaded by the system libraries. This only matters when
*/ // using the component build, where our SQLite's symbols are visible to the
/* TODO(shess): Consider using build/build_config.h for OS_ macros. */ // dynamic library loader.
/* TODO(shess): build_config.h uses unistd.h, perhaps for portability reasons, #include "third_party/sqlite/amalgamation/rename_exports.h"
* but AFAICT there are no current portability concerns here. limits.h is
* another alternative. // Linux-specific configuration fixups.
*/ #if defined(__linux__)
// features.h, included below, indirectly includes sys/mman.h. The latter header // features.h, included below, indirectly includes sys/mman.h. The latter header
// only defines mremap if _GNU_SOURCE is defined. Depending on the order of the // only defines mremap if _GNU_SOURCE is defined. Depending on the order of the
// files in the amalgamation, removing the define below may result in a build // files in the amalgamation, removing the define below may result in a build
// error on Linux. // error on Linux.
#if defined(__GNUC__) && !defined(_GNU_SOURCE) #if defined(__GNUC__) && !defined(_GNU_SOURCE)
# define _GNU_SOURCE #define _GNU_SOURCE
#endif #endif
#include <features.h> #include <features.h>
/* SQLite wants to track malloc sizes. On OSX it uses malloc_size(), on // SQLite wants to track malloc sizes. On OSX it uses malloc_size(), on Windows
* Windows _msize(), elsewhere it handles it manually by enlarging the malloc // _msize(), elsewhere it handles it manually by enlarging the malloc and
* and injecting a field. Enable malloc_usable_size() for Linux. // injecting a field. Enable malloc_usable_size() for Linux.
* //
* malloc_usable_size() is not exported by the Android NDK. It is not // malloc_usable_size() is not exported by the Android NDK. It is not
* implemented by uclibc. // implemented by uclibc.
*/ #if !defined(__UCLIBC__) && !defined(__ANDROID__)
#if defined(__linux__) && !defined(__UCLIBC__)
#define HAVE_MALLOC_H 1 #define HAVE_MALLOC_H 1
#define HAVE_MALLOC_USABLE_SIZE 1 #define HAVE_MALLOC_USABLE_SIZE 1
#endif #endif
/* TODO(shess): Eat other config options from gn and gyp? */ #endif // defined(__linux__)
#endif // THIRD_PARTY_SQLITE_AMALGAMATION_CONFIG_H_
This diff is collapsed.
This diff is collapsed.
This diff is collapsed.
...@@ -9,10 +9,19 @@ cd src ...@@ -9,10 +9,19 @@ cd src
mkdir bld mkdir bld
cd bld cd bld
../configure ../configure
FILES="shell.c sqlite3.h sqlite3.c"
OPTS="" OPTS=""
make "OPTS=$OPTS" $FILES make "OPTS=$OPTS" shell.c sqlite3.h sqlite3.c
cp -f $FILES ../../amalgamation cp -f sqlite3.h sqlite3.c ../../amalgamation
# shell.c must be placed in a different directory from sqlite3.h, because it
# contains an '#include "sqlite3.h"' that we want to resolve to our custom
# //third_party/sqlite/sqlite3.h, not to the sqlite3.h produced here.
mkdir -p ../../amalgamation/shell/
cp -f shell.c ../../amalgamation/shell/
cd .. cd ..
rm -rf bld rm -rf bld
../scripts/extract_sqlite_api.py ../amalgamation/sqlite3.h \
../amalgamation/rename_exports.h
\ No newline at end of file
...@@ -4,15 +4,21 @@ ...@@ -4,15 +4,21 @@
#ifndef THIRD_PARTY_SQLITE_SQLITE3_H_ #ifndef THIRD_PARTY_SQLITE_SQLITE3_H_
#define THIRD_PARTY_SQLITE_SQLITE3_H_ #define THIRD_PARTY_SQLITE_SQLITE3_H_
#pragma once
// This is a shim header to include the right sqlite3 header. // This is a shim header to include the right sqlite3 headers.
// Use this instead of referencing the sqlite3 header directly. // Use this instead of referencing sqlite3 headers directly.
#if defined(USE_SYSTEM_SQLITE) #if defined(USE_SYSTEM_SQLITE)
#include <sqlite3.h> #include <sqlite3.h>
#else #else
// We prefix chrome_ to SQLite's exported symbols, so that we don't clash with
// other SQLite libraries loaded by the system libraries. This only matters when
// using the component build, where our SQLite's symbols are visible to the
// dynamic library loader.
#include "third_party/sqlite/amalgamation/rename_exports.h"
#include "third_party/sqlite/amalgamation/sqlite3.h" #include "third_party/sqlite/amalgamation/sqlite3.h"
#endif
#endif // defined(USE_SYSTEM_SQLITE)
#endif // THIRD_PARTY_SQLITE_SQLITE3_H_ #endif // THIRD_PARTY_SQLITE_SQLITE3_H_
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment