Write support for tables implemented from extensions #4094

alessandrogario · 2018-01-31T19:03:22Z

Description

This PR adds support for writable extension tables by providing an xUpdate sqlite3 implementation.

Disclamer

Write support for core/built-in tables is NOT implemented, and will not work.

Demo

This is a small demo for the following example: osquery/examples/example_writable_table.cpp

osquery> select * from WritableTable;
osquery> insert into WritableTable (text, integer) values ("1", 1);
osquery> insert into WritableTable (text, integer) values ("1", 1);
Error: constraint failed
osquery> insert into WritableTable (text, integer) values ("2", 2);
osquery> select * from WritableTable;
+------+---------+
| text | integer |
+------+---------+
| 2    | 2       |
| 1    | 1       |
+------+---------+
osquery> delete from WritableTable where text = "1";
osquery> select * from WritableTable;
+------+---------+
| text | integer |
+------+---------+
| 2    | 2       |
+------+---------+
osquery> insert into WritableTable (text, integer) values ("3", 3);
osquery> select * from WritableTable;
+------+---------+
| text | integer |
+------+---------+
| 3    | 3       |
| 2    | 2       |
+------+---------+
osquery> update WritableTable set text="2", integer=2 where text="3";
Error: constraint failed
osquery> update WritableTable set text="5", integer=5 where text="3";
osquery> select * from WritableTable;
+------+---------+
| text | integer |
+------+---------+
| 5    | 5       |
| 2    | 2       |
+------+---------+

facebook-github-bot · 2018-01-31T20:30:05Z

@alessandrogario has updated the pull request.

obelisk · 2018-01-31T21:25:16Z

This is really interesting, what potential use cases do you envision for this sort of thing?

obelisk

I've marked a few things as what I think are API Changes which might break other people's building of things that use the osquery SDK (at least I think that's what would happen). This is not a huge problem but it is something we want to make sure we're not doing flippantly.

Overall I like this a lot, this is going to be a super cool feature to have and will provide a lot of value.

Thanks!

obelisk · 2018-02-01T01:22:15Z

osquery/examples/example_writable_table.cpp

+    }
+
+    // We may have to update our rowid
+    std::string new_rowid;


Nit. Could use a ternary here.

obelisk · 2018-02-01T01:22:15Z

osquery/examples/example_writable_table.cpp

+  /// Callback for DELETE queries
+  QueryData delete_(QueryContext& context, const PluginRequest& request) {
+    auto rowid = request.at("id");
+    if (id_to_internal_id.count(rowid) == 0) {


Prefer find. Then you will have a pointer to the item to use below.

Yeah, you are right! I'm not sure why but I ended up matching some other code I found in the other files without thinking! Going to fix it right now

obelisk · 2018-02-01T01:22:15Z

osquery/examples/example_writable_table.cpp

+  // Callback for UPDATE queries
+  QueryData update(QueryContext& context, const PluginRequest& request) {
+    std::string current_rowid = request.at("id");
+    if (id_to_internal_id.count(current_rowid) == 0) {


Prefer find.

obelisk · 2018-02-01T01:22:15Z

osquery/examples/example_writable_table.cpp

+    RowData user_row_data;
+    auto column_index = 0U;
+
+    for (std::string value; std::getline(stream, value, ';');) {


Much of this logic is duplicated in the insert function. Is there a way we can utilize that so that if/when this code is updated there is a smaller chance of error (updating one but not the other)?

obelisk · 2018-02-01T01:22:15Z

osquery/examples/example_writable_table.cpp

+
+  /// Callback for DELETE queries
+  QueryData delete_(QueryContext& context, const PluginRequest& request) {
+    auto rowid = request.at("id");


Can be const &?

obelisk · 2018-02-01T01:22:15Z

osquery/sql/virtual_table.cpp

+  sqlite3_module* module =
+      tables::sqlite::getVirtualTableModule(name, is_extension);
+  if (module == nullptr) {
+    auto error_message =


I would just do this in the VLOG(1). In my opinion, the only time that returning an error message this verbose in a Status is useful is if you are checking it in code, or are going to print it out. Since you've already printed it out, you probably don't need to return it and Status(1) should suffice.

cc: @muffins Do you agree with this as a design decision? Please tell me I'm wrong if you think so.

obelisk · 2018-02-01T01:22:15Z

include/osquery/tables.h

@@ -752,7 +777,7 @@ class TablePlugin : public Plugin {

 protected:
  /// An SQL table containing the table definition/syntax.
-  std::string columnDefinition() const;
+  std::string columnDefinition(bool is_extension) const;


This change could break other people who are compiling extensions against this header correct? Could we provide default values instead?

The columnDefinition method was defined several times (as a method and as a function) both overloaded and with default parameters, and this made it hard to read because you never knew where the call was going when passing some parameters (and this is why I didn't provide a default value here).

Isn't this method only used by osquery and not by the extension developers? (I'm asking because I didn't have to change the other example extensions after this patch).

It is possible that other people are using them because they are in our public headers. This (and the changes I marked API Change) could in theory break anyone who is using the SDK. If this is the only way to do it, or this is a great improvement in the API then we can merge this but we shouldn't do so without evaluating the possibility of breaking some third party extensions.

obelisk · 2018-02-01T01:22:15Z

include/osquery/tables.h

@@ -846,10 +871,6 @@ class TablePlugin : public Plugin {
  static void setRequestFromContext(const QueryContext& context,
                                    PluginRequest& request);

-  /// Helper data structure transformation methods.
-  static void setContextFromRequest(const PluginRequest& request,


I moved it because it wasn't referenced anywhere! Thanks for spotting this!

obelisk · 2018-02-01T01:22:15Z

include/osquery/tables.h

  FRIEND_TEST(VirtualTableTests, test_tableplugin_statement);
  FRIEND_TEST(VirtualTableTests, test_indexing_costs);
  FRIEND_TEST(VirtualTableTests, test_table_results_cache);
  FRIEND_TEST(VirtualTableTests, test_yield_generator);
 };

 /// Helper method to generate the virtual table CREATE statement.
-std::string columnDefinition(const TableColumns& columns);
+std::string columnDefinition(const TableColumns& columns, bool is_extension);


API Change.

obelisk · 2018-02-01T01:22:15Z

include/osquery/tables.h


 /// Helper method to generate the virtual table CREATE statement.
 std::string columnDefinition(const PluginResponse& response,
-                             bool aliases = false);
+                             bool aliases,


API Change.

facebook-github-bot · 2018-02-01T11:07:47Z

@alessandrogario has updated the pull request. View: changes

facebook-github-bot · 2018-02-01T11:17:03Z

@alessandrogario has updated the pull request.

facebook-github-bot · 2018-02-01T14:05:33Z

@alessandrogario has updated the pull request.

obelisk · 2018-02-01T20:25:18Z

osquery/examples/example_writable_table.cpp

+    row_data.rowid = new_rowid;
+
+    // Build the primary key and make sure it's unique
+    std::string internal_rowid =


This should be abstracted as a function that generates the primary key of the table. Currently adding a new column to the primary key is going to require changes to places and will be error prone (even if you use all the right columns but in the wrong order).

obelisk · 2018-02-01T20:25:18Z

osquery/examples/example_writable_table.cpp

+    }
+
+    // Build the primary key and make sure it's unique
+    std::string new_internal_id =


Call new primary key function.

obelisk · 2018-02-01T20:25:18Z

osquery/examples/example_writable_table.cpp

+
+    auto current_internal_id = internal_id_it->second;
+
+    // This variable contains a value for each column in the table, separated by


Is this still true?

obelisk · 2018-02-01T20:25:18Z

osquery/examples/example_writable_table.cpp

+    auto new_rowid_it = request.find("new_id");
+
+    std::string new_rowid;
+    if (new_rowid_it != request.end()) {


This could be a ternary.

std::string new_rowid = new_rowid_it != request.end() ? new_rowid_it->second : current_rowid;

obelisk · 2018-02-01T20:25:18Z

osquery/examples/example_writable_table.cpp

+
+  /// Callback for INSERT queries
+  QueryData insert(QueryContext& context, const PluginRequest& request) {
+    // This variable contains a value for each column in the table, separated by


Is this still true?

obelisk · 2018-02-01T20:25:18Z

osquery/examples/example_writable_table.cpp

+    QueryData results;
+
+    for (const auto& row_descriptor : row_map) {
+      const auto row_data = row_descriptor.second;


I think this can be a reference

facebook-github-bot · 2018-02-01T21:45:46Z

@alessandrogario has updated the pull request.

facebook-github-bot · 2018-02-01T21:50:28Z

@alessandrogario has updated the pull request. View: changes

facebook-github-bot · 2018-02-01T21:55:46Z

@alessandrogario has updated the pull request. View: changes

obelisk · 2018-02-01T22:35:26Z

osquery/core/tables.cpp

+    response = update(context, request);
+
+  } else if (action == "columns") {
+    static_cast<void>(request);


What does this do?

This is just to make static analysis (and warnings with -Wunused) happy; I kind of keep adding it due to habits for other projects! I can see it's not applicable here, I'll remove it!

obelisk · 2018-02-01T22:35:26Z

osquery/examples/example_writable_table.cpp

+class WritableTable : public TablePlugin {
+ private:
+  /// Contents for a single row
+  struct RowData final {


Can you elaborate on why you chose to use a struct here instead of an unordered_map? It seems to me that if these were maps you could just return them in your gen function instead of copying the values to the Row object. Maybe you can just use Row objects as well? Another upside is that most of your int conversions will go away.

The only draw back I see is not being able to store types in the their native types (they all become strings).

Let me know if I missed something obvious about your implementation :p

I couldn't use strings because the data is now serialized using rapidjson, automatically matching the type of each column

obelisk · 2018-02-01T22:35:26Z

osquery/examples/example_writable_table.cpp

+  }
+
+  /// Expands a value list returned by osquery into a RowData structure
+  bool getRowData(RowData& row_data, const std::string& value_list) const {


This should return a status

obelisk · 2018-02-01T22:35:26Z

osquery/examples/example_writable_table.cpp

+    rapidjson::Document document;
+    document.Parse(value_list);
+    if (document.HasParseError() || !document.IsArray()) {
+      VLOG(1) << "Invalid format";


Put these into the message of the status

obelisk · 2018-02-01T22:35:26Z

osquery/examples/example_writable_table.cpp

+    }
+
+    if (document.Size() != 2U) {
+      VLOG(1) << "Wrong column count.";


obelisk · 2018-02-01T22:35:26Z

osquery/examples/example_writable_table.cpp

+      row_data.text = document[0].GetString();
+    }
+
+    if (document[0].IsNull()) {


Nit. Could be ternary.

obelisk · 2018-02-01T22:35:38Z

ok to test

osqueryer · 2018-02-01T22:54:26Z

👎 The commit d10a5ee (Job results: 3349) failed one or more tests (Windows).

osqueryer · 2018-02-01T23:03:05Z

👎 The commit d10a5ee (Job results: 3350) failed one or more tests (Windows).

facebook-github-bot · 2018-02-02T15:32:49Z

@alessandrogario has updated the pull request. View: changes

osqueryer · 2018-02-02T15:52:00Z

👎 The commit b4eb518 (Job results: 3352) failed one or more tests (Windows).

osqueryer · 2018-02-02T15:57:23Z

👎 The commit b4eb518 (Job results: 3353) failed one or more tests (Windows).

fmanco · 2018-02-02T22:39:57Z

Hi @alessandrogario, why is it you say "Write support for core/built-in tables is NOT implemented, and will not work."?

obelisk · 2018-02-02T23:55:40Z

@fmanco Building in tables won't work because the code is generated for those tables early in the build process without the insert method set. We would have to change a lot more code and create some more context to support this in core tables.

facebook-github-bot · 2018-02-03T14:04:19Z

@alessandrogario has updated the pull request. View: changes

obelisk · 2018-02-05T09:27:10Z

osquery/examples/example_writable_table.cpp

+  }
+
+  /// Saves the given row
+  Status saveRow(const Row& row, PrimaryKey primary_key = std::string()) {


I would think that saveRow should check the primary key doesn't already exist, otherwise if you add saveRow in another place, you need to duplicate that logic.

I need to keep them separate so that the statements can be blocked earlier (i.e. without modifying the current data or calling the rowid generator) in case of a constraint failure. If I do this check inside the saveRow call, then I would have to undo changes

obelisk · 2018-02-05T09:27:10Z

osquery/examples/example_writable_table.cpp

+
+  /// Expands a value list returned by osquery into a Row (without the rowid
+  /// column)
+  Status getRowData(Row& row, const std::string& value_list) const {


Nit. We are no longer using a value list, we should rename this to JSON because that’s what it is.

I kept value_list because it is more descriptive than just 'json' (it is in fact a list of values, regardless of the format). Maybe we can use json_value_array

obelisk · 2018-02-05T09:27:10Z

osquery/examples/example_writable_table.cpp

+
+    row["text"] = document[0].IsNull() ? "" : document[0].GetString();
+
+    auto value = document[1].IsNull() ? 0 : document[1].GetInt();


Combine these two lines so we can get rid of the value intermediate variable.

Duh! Was trying to keep short lines :D Will fix right now!

obelisk · 2018-02-05T09:27:10Z

osquery/examples/example_writable_table.cpp

+
+    // Finally, save the row; also pass the primary key we calculated so that
+    // the function doesn't have to compute it again
+    status = saveRow(row, primary_key);


If we fold the primary key check logic in here we can combine the error checking here and on line 152.

See #4094 (comment)

obelisk · 2018-02-05T09:27:10Z

osquery/sql/virtual_table.cpp

+std::vector<std::string> extension_table_list;
+std::mutex extension_table_list_mutex;
+
+bool isExtensionTable(const std::string& table_name) {


Do we need a new array to keep track of this or can we use the registry?

I tried to use the registry many times, but it doesn't look like it can be used reliably while it is initializing the tables

obelisk · 2018-02-05T09:27:10Z

osquery/sql/virtual_table.cpp

@@ -178,13 +488,20 @@ int xCreate(sqlite3* db,
    return SQLITE_ERROR;
  }

+  // Tables implemented from extensions can be made read/write if they overwrite


S/overwrite/implement

obelisk · 2018-02-05T09:27:10Z

osquery/sql/virtual_table.cpp

+
+struct sqlite3_module* getVirtualTableModule(const std::string& table_name,
+                                             bool extension) {
+  static std::unordered_map<std::string, struct sqlite3_module> modules;


I don't like this static definition, I think it makes the code messier.

cc @muffins Thoughts on this?

This is done to actually avoid making the code messier (because we are reducing the scope as much as possible since it is not needed outside the dedicated interface function). This is also thread safe (as long as we compile with C++11 or better).

I can move this outside and remove the static if you prefer

Makes sense :)

Hi @alessandrogario I believe initialization 'static std::unordered_map<std::string, struct sqlite3_module> modules;' is thread safe but 'modules[table_name] = {};' is not thead safe
I.e. initialization of a function local static object is thread safe not the operations on them.

Yes! I used the wrong terms (I meant that this construct is useful and use for singletons for example); initialization is performed on a single thread (you can see the original code doesn't protect access to this variable)

In the present code usage of variable 'module' is thread safe since -

It is initialized as static variable.

After initialization it is being used as read-only.
While the code you are proposing 'modules' variable is not read-only.

You are right it is not accessed in read only, but I was under the assumption that initialization was single threaded.

EDIT: Taking a look at it again, and it does seem that everything is performed in a unique call from a single thread, so it doesn't matter whether you lock it or not. I will move the function call under the attachLock call just to be sure. What do you think?

Thanks!

"it does seem that everything is performed in a unique call from a single thread, so it doesn't matter whether you lock it or not."
I observed that the code is written with keeping thread safety in mind. Until now access to module object was being done in thread safe manner since it is being accessed as read-only. At present only single thread of control is accessing this code but it may not be the case in future.

I believe attachlock will not be enough since it is synchronizing access to instance but modules object can be accessed across the instances. Please correct me if I am wrong.

You may be right! I've ended up reverting the last commit, and adding a mutex. Thanks!

facebook-github-bot · 2018-02-05T10:48:13Z

@alessandrogario has updated the pull request. View: changes

osqueryer · 2018-02-05T11:21:54Z

👎 The commit 312e938 (Job results: 3358) failed one or more tests (Windows).

alessandrogario · 2018-02-05T12:40:09Z

I'm not really sure why osqueryer commented that the test has failed; it is passing on my virtual machine, and it is not shown as failing in the check list at the bottom of the page!

osqueryer · 2018-04-03T14:41:43Z

👎 The commit 45b582e (Job results: 6798) failed one or more tests (Linux).

osqueryer · 2018-04-03T14:43:50Z

👎 The commit 45b582e (Job results: 3693) failed one or more tests (Windows).

osqueryer · 2018-04-03T14:48:27Z

👎 The commit 45b582e (Job results: 6799) failed one or more tests (Linux).

facebook-github-bot · 2018-04-03T14:59:05Z

@alessandrogario has updated the pull request.

osqueryer · 2018-04-03T15:06:19Z

👎 The commit 967a99c (Job results: 6800) failed one or more tests (Linux).

osqueryer · 2018-04-03T15:08:38Z

👎 The commit 967a99c (Job results: 6801) failed one or more tests (Linux).

facebook-github-bot · 2018-04-04T12:25:13Z

@alessandrogario has updated the pull request.

osqueryer · 2018-04-04T12:36:20Z

👎 The commit 48f5e7f (Job results: 1267) failed one or more tests (FreeBSD).

osqueryer · 2018-04-04T12:39:01Z

👎 The commit 48f5e7f (Job results: 1268) failed one or more tests (FreeBSD).

facebook-github-bot · 2018-05-01T12:51:39Z

@alessandrogario has updated the pull request. View: changes

facebook-github-bot · 2018-05-16T11:10:12Z

@alessandrogario has updated the pull request.

theopolis

Ok, I left myself some pointers for a larger review later.

I think this approach works. I want to be extra-sure about the keys (their names) used in serialization.

theopolis · 2018-06-01T21:36:14Z

osquery/sql/sqlite_util.cpp

@@ -192,12 +192,16 @@ Status SQLiteSQLPlugin::attach(const std::string& name) {
    return status;
  }

-  auto statement = columnDefinition(response);
+  bool is_extension = true;
+  auto statement = columnDefinition(response, false, is_extension);


Confirm that this attach method is only called for external tables.

theopolis · 2018-06-01T21:36:14Z

osquery/sql/virtual_table.cpp

@@ -702,14 +1094,16 @@ void attachVirtualTables(const SQLiteDBInstanceRef& instance) {
  }

  PluginResponse response;
+  bool is_extension = false;


TODO(Ted): Double check this.

theopolis · 2018-06-01T21:36:14Z

osquery/core/tables.cpp

+  // Tables implemented by extension can be made read/write; make sure to always
+  // keep the rowid column; this makes them easier to manage, and also removes
+  // any limit on the primary key count
+  if (is_extension) {


Let's see if we can fix the INDEX=True, so we wont need this bool.

theopolis · 2018-06-01T21:36:14Z

osquery/sql/virtual_table.cpp

+std::mutex extension_table_list_mutex;
+
+bool isExtensionTable(const std::string& table_name) {
+  std::lock_guard<std::mutex> locker(extension_table_list_mutex);


You can use ReadLock lock(...);

theopolis · 2018-06-01T21:36:14Z

osquery/sql/virtual_table.cpp

+
+struct sqlite3_module* getVirtualTableModule(const std::string& table_name,
+                                             bool extension) {
+  std::lock_guard<std::mutex> lock(sqlite_module_map_mutex);


nb: ReadLock lock...;

theopolis · 2018-06-01T21:36:14Z

osquery/sql/virtual_table.cpp

+
+  std::string table_name = pVtab->content->name;
+
+  if (FLAGS_table_delay > 0 && pVtab->instance->tableCalled(content)) {


You can delete this.

theopolis · 2018-06-01T21:36:14Z

osquery/sql/virtual_table.cpp

+    // Apply an optional sleep between table calls.
+    sleepFor(FLAGS_table_delay);
+  }
+  pVtab->instance->addAffectedTable(content);


You can delete this.

theopolis · 2018-06-01T21:36:14Z

osquery/sql/virtual_table.cpp

+
+  // The SQLite instance communicates to the TablePlugin via the context.
+  QueryContext context(content);
+  context.useCache(pVtab->instance->useCache());


You can delete this.

theopolis · 2018-06-01T21:36:14Z

osquery/sql/virtual_table.cpp

+      return serializerError;
+    }
+
+    plugin_request.insert({"json_value_array", json_value_array});


Investigate the use of this map key, why json_value_array?

theopolis · 2018-06-01T21:36:14Z

osquery/sql/virtual_table.cpp

+      return serializerError;
+    }
+
+    plugin_request.insert({"json_value_array", json_value_array});


akindyakov · 2018-07-12T15:59:13Z

include/osquery/tables.h

+  /// Callback for DELETE statements
+  virtual QueryData delete_(QueryContext& context,
+                            const PluginRequest& request) {
+    static_cast<void>(context);


May I ask you to use in such cases boost::ignore_unused as more explicit way to switch off this warning. ref

akindyakov · 2018-07-12T15:59:13Z

osquery/devtools/shell.cpp

+    "list",
+    "semi",
+    "csv",
+    "pretty",


May I ask you to submit formatting changes in separate PR?

This sometimes happens when we update the clang-format executable from the brew repository and there are changes on how code is formatted by the tool. If I remove this, then the make audit step will fail (and the CI workers will fail too).

These lines will disappear once we rebase on master after the correctly formatted code is committed.

As far as I know make audit checks only changed lines. If some particular code is not satisfy clang-format and is not changed the make audit will not complain about it.
For instance:

$ git checkout -b test $ make audit

And disregarding to the wrong formatting in shell.cpp audit is not complain about it. But if you change one letter in this string - it will start complain.

There is a instrument to avoid complains of audit test in changed code: make format_master. It get a diff between local master and feature brach and fix up format only in changed lines.

That's really cool! Has this been introduced recently? I had several PR fail due to this in the recent past!

akindyakov · 2018-07-12T15:59:13Z

osquery/devtools/shell.cpp

@@ -1083,7 +1088,7 @@ inline void meta_schema(int nArg, char** azArg) {
      fprintf(stdout,
              "CREATE TABLE %s%s;\n",
              table.c_str(),
-              osquery::columnDefinition(response, true).c_str());
+              osquery::columnDefinition(response, true, false).c_str());


I'd like to suggest you make such calls more explicit in terms of passing arguments. Smth like this:

auto const aliases = true; auto const is_extension = false; osquery::columnDefinition(response, aliases, is_extension).c_str());

What do you think?

akindyakov · 2018-07-12T15:59:13Z

osquery/sql/virtual_table.cpp

+  return (std::find(extension_table_list.begin(),
+                    extension_table_list.end(),
+                    table_name) != extension_table_list.end());
+}


There are 3 things that are used only together, would you like to make a make a class for them?

There is no usage of it outside of cpp file. Would you like to put it under anonymous namespace?

As far as order of elements in extension_table_list is not important (I didn't find it, may be I'm wrong) I'd suggest to use unordered_set instead of it.

akindyakov · 2018-07-12T15:59:13Z

osquery/sql/virtual_table.cpp

+
+bool getColumnValue(std::string& value,
+                    int index,
+                    int argc,


index and argc are used for indexing, would you like to define them as std::size_t? To avoid a problems with negative values.

See #4094 (comment)

akindyakov · 2018-07-12T15:59:13Z

osquery/sql/virtual_table.cpp

+
+    auto buffer_size = sqlite3_value_bytes(sqlite_value);
+    value.resize(buffer_size);
+    std::memcpy(&value[0], data_pointer, buffer_size);


You could do the same operation without using low level function memcpy.
Smth like this:

value.assign(data_pointer, buffer_size);

akindyakov · 2018-07-12T15:59:13Z

osquery/sql/virtual_table.cpp

+  }
+
+  case SQLITE_NULL: {
+    break;


Should we put in the value something like "0" or similar in this case?

The value variable is cleared before we enter the switch, so in case of a NULL we get an empty string. This is how NULL values are generally handled by tables

akindyakov · 2018-07-12T15:59:13Z

osquery/sql/virtual_table.cpp

+
+// Type-aware serializer to pass parameters between osquery and the extension
+int serializeUpdateParameters(std::string& json_value_array,
+                              int argc,


Would you like to consider of using std::size_t for argc variable?
And also define it as const?

See #4094 (comment)

akindyakov · 2018-07-12T15:59:13Z

osquery/sql/virtual_table.cpp

+                              int argc,
+                              sqlite3_value** argv,
+                              const TableColumns& columnDescriptors) {
+  if (columnDescriptors.size() != static_cast<size_t>(argc - 2)) {


It could be problematic to substitute something from number and cast it to unsigned type. Is here any possibility for argc to be less than 2?

All the argc parameters are defined as int to match the type used by sqlite3 to define that variable.

If we switch the type to something else, we will have to cast it to size_t when we receive it and back to int when we return it back to the library (would you prefer this approach?).

Parameter serialization only happens when an UPDATE or INSERT statement has been received.

This is a short explanation of how it works for this case, but the documentation goes in more detail:

Update queries

We are updating an existing row; the ID for that row is inside argv[0]

argv[1] == NULL

The table code is expected to keep using the ID associated with the row being modified. This ID can be found in argv[0].

argv[1] == INTEGER

The table handler is expected to update the row, invalidate the existing row ID (argv[0]) and use the new one found in argv[1].

Insert queries

In this case, the row is new so there is no existing row ID. This means that argv[0] will be set to SQLITE_NULL.

argv[1] == NULL

The table is expected to come up with a new row ID and return it back to sqlite.

argv[1] == INTEGER

sqlite requires the table logic to use the supplied row ID when saving the row.

Thank you for the explanation. I see your point. But even so I'd like to suggest you to use a std::size_t here.

int in sqlite3 interface is just a part of interface. And is good to see this conversion from std::size_t to int only when we use the sqlite library.

Mixing up a signed and unsigned types is bad idea. It could be cause for many bugs and even UB, which is pure evil. Look at the example:

#include <iostream> int main() { const auto sv = -2; const auto usv = 1u; auto res = sv + usv; std::cout << res << '\n'; return 0; }

What is the type of res and what is the answer? This is potential UB.
3. You right for high load pieces of code static conversion from one type to another could be have a notable performance penalty. But we can not solve this problem by using a int type here. Because there still will be a lot of conversions. And, that is the worst, they will be implicit. Which is hard to find, to control, test and debug.
4. And semantics, std::size_t is a special type dedicated to represent indexes, address offsets and so on. If you see that type you will know - that variable is for the size. int is too common, variable of this type could be anything.

akindyakov · 2018-07-12T15:59:13Z

osquery/sql/virtual_table.cpp

+      break;
+    }
+
+    default: { return SQLITE_MISMATCH; }


Should we write something to LOG(ERROR) here?

muffins · 2018-07-15T21:05:04Z

@alessandrogario looks like @theopolis and @akindyakov had a few more things to fix, and this needs a rebase, but we should be getting close on this yeah?

alessandrogario · 2018-07-16T14:04:40Z

Hey @muffins!

I've rebased and fixed the issues that have been mentioned; the remaining comments are personal notes/bookmarks that Ted has left for his review during QueryCon. I don't think he is requesting for any change, but correct me if I'm wrong!

Dismissing as the review is stale.

muffins

LGTM. Let's use smaller follow-up PRs for any additionally changes folks would like to see, but for now I think we're ok bringing this in and iterating.

…y#4094)

facebook-github-bot added the cla signed label Jan 31, 2018

obelisk added feature extensions labels Jan 31, 2018

obelisk requested changes Feb 1, 2018

View changes

obelisk added the API change label Feb 1, 2018

obelisk requested changes Feb 1, 2018

View changes

obelisk added the needs response label Feb 1, 2018

obelisk reviewed Feb 1, 2018

View changes

obelisk removed the API change label Feb 1, 2018

obelisk reviewed Feb 5, 2018

View changes

theopolis reviewed Jun 1, 2018

View changes

akindyakov reviewed Jul 12, 2018

View changes

muffins added this to the 3.3.0 milestone Jul 15, 2018

muffins added needs response and removed ready for review labels Jul 15, 2018

alessandrogario added 6 commits Jul 3, 2018

Add write support to extension tables

dbb5419

Add custom error messages; default to read-only errors

9e7f416

Code review changes

0ca315f

Code review changes

d26dd9d

Code review changes

ef9c952

Code review changes

Loading status checks…

9c8d349

muffins approved these changes Jul 17, 2018

View changes

muffins merged commit 8fe570b into osquery:master Jul 17, 2018
5 checks passed

5 checks passed

Code Audit Build finished.
Details

FreeBSD Build finished.
Details

Linux Build finished.
Details

Windows Build finished.
Details

macOS/OS X Build finished.
Details

alessandrogario deleted the alessandro/feature/xupdate-support branch Jul 17, 2018

trizt added a commit to trizt/osquery that referenced this pull request May 24, 2019

feature: Write support for tables implemented from extensions (osquer…

2a68489

…y#4094)


		auto current_internal_id = internal_id_it->second;

		// This variable contains a value for each column in the table, separated by


		row["text"] = document[0].IsNull() ? "" : document[0].GetString();

		auto value = document[1].IsNull() ? 0 : document[1].GetInt();


		std::string table_name = pVtab->content->name;

		if (FLAGS_table_delay > 0 && pVtab->instance->tableCalled(content)) {

osquery / osquery

Join GitHub today

Write support for tables implemented from extensions #4094

Write support for tables implemented from extensions #4094

Conversation

alessandrogario commented Jan 31, 2018

Description

Disclamer

Demo

facebook-github-bot commented Jan 31, 2018

obelisk commented Jan 31, 2018

obelisk left a comment

This comment has been minimized.

obelisk Feb 1, 2018 Contributor

This comment has been minimized.

obelisk Feb 1, 2018 Contributor

This comment has been minimized.

alessandrogario Feb 1, 2018 Author Contributor

This comment has been minimized.

obelisk Feb 1, 2018 Contributor

This comment has been minimized.

obelisk Feb 1, 2018 Contributor

This comment has been minimized.

obelisk Feb 1, 2018 Contributor

This comment has been minimized.

obelisk Feb 1, 2018 Contributor

This comment has been minimized.

obelisk Feb 1, 2018 Contributor

This comment has been minimized.

alessandrogario Feb 1, 2018 Author Contributor

This comment has been minimized.

obelisk Feb 1, 2018 Contributor

This comment has been minimized.

obelisk Feb 1, 2018 Contributor

This comment has been minimized.

alessandrogario Feb 1, 2018 Author Contributor

This comment has been minimized.

obelisk Feb 1, 2018 Contributor

This comment has been minimized.

obelisk Feb 1, 2018 Contributor

facebook-github-bot commented Feb 1, 2018

facebook-github-bot commented Feb 1, 2018

facebook-github-bot commented Feb 1, 2018

This comment has been minimized.

obelisk Feb 1, 2018 Contributor

This comment has been minimized.

obelisk Feb 1, 2018 Contributor

This comment has been minimized.

obelisk Feb 1, 2018 Contributor

This comment has been minimized.

obelisk Feb 1, 2018 Contributor

This comment has been minimized.

obelisk Feb 1, 2018 Contributor

This comment has been minimized.

obelisk Feb 1, 2018 Contributor

facebook-github-bot commented Feb 1, 2018

facebook-github-bot commented Feb 1, 2018

facebook-github-bot commented Feb 1, 2018

This comment has been minimized.

obelisk Feb 1, 2018 Contributor

This comment has been minimized.

alessandrogario Feb 1, 2018 Author Contributor

This comment has been minimized.

obelisk Feb 1, 2018 Contributor

This comment has been minimized.

alessandrogario Feb 1, 2018 Author Contributor

This comment has been minimized.

obelisk Feb 1, 2018 Contributor

This comment has been minimized.

obelisk Feb 1, 2018 Contributor

This comment has been minimized.

obelisk Feb 1, 2018 Contributor

This comment has been minimized.

obelisk Feb 1, 2018 Contributor

obelisk commented Feb 1, 2018

osqueryer commented Feb 1, 2018

osqueryer commented Feb 1, 2018

facebook-github-bot commented Feb 2, 2018

osqueryer commented Feb 2, 2018

osqueryer commented Feb 2, 2018

obelisk Feb 1, 2018
Contributor

obelisk Feb 1, 2018
Contributor

alessandrogario Feb 1, 2018
Author Contributor

obelisk Feb 1, 2018
Contributor

obelisk Feb 1, 2018
Contributor

obelisk Feb 1, 2018
Contributor

obelisk Feb 1, 2018
Contributor

obelisk Feb 1, 2018
Contributor

alessandrogario Feb 1, 2018
Author Contributor

obelisk Feb 1, 2018
Contributor

obelisk Feb 1, 2018
Contributor

alessandrogario Feb 1, 2018
Author Contributor

obelisk Feb 1, 2018
Contributor

obelisk Feb 1, 2018
Contributor

obelisk Feb 1, 2018
Contributor

obelisk Feb 1, 2018
Contributor

obelisk Feb 1, 2018
Contributor

obelisk Feb 1, 2018
Contributor

obelisk Feb 1, 2018
Contributor

obelisk Feb 1, 2018
Contributor

obelisk Feb 1, 2018
Contributor

alessandrogario Feb 1, 2018
Author Contributor

obelisk Feb 1, 2018
Contributor

alessandrogario Feb 1, 2018
Author Contributor

obelisk Feb 1, 2018
Contributor

obelisk Feb 1, 2018
Contributor

obelisk Feb 1, 2018
Contributor

obelisk Feb 1, 2018
Contributor

obelisk Feb 5, 2018
Contributor

alessandrogario Feb 5, 2018
Author Contributor

obelisk Feb 5, 2018
Contributor

alessandrogario Feb 5, 2018
Author Contributor

obelisk Feb 5, 2018
Contributor

alessandrogario Feb 5, 2018
Author Contributor

obelisk Feb 5, 2018
Contributor

alessandrogario Feb 5, 2018
Author Contributor

obelisk Feb 5, 2018
Contributor

alessandrogario Feb 5, 2018
Author Contributor

obelisk Feb 5, 2018
Contributor

obelisk Feb 5, 2018
Contributor

alessandrogario Feb 5, 2018
Author Contributor

obelisk Feb 5, 2018
Contributor

uptycs-nishant Feb 5, 2018
Contributor

alessandrogario Feb 7, 2018 •

edited

Author Contributor

uptycs-nishant Feb 9, 2018
Contributor

alessandrogario Feb 15, 2018 •

edited

Author Contributor

uptycs-nishant Feb 19, 2018
Contributor

alessandrogario Feb 23, 2018
Author Contributor

theopolis Jun 1, 2018
Member

theopolis Jun 1, 2018
Member

theopolis Jun 1, 2018
Member

theopolis Jun 1, 2018
Member

theopolis Jun 1, 2018
Member