Fix data type macro used for 64-bit timestamp variables by mike-myers-tob · Pull...
source link: https://github.com/osquery/osquery/pull/6897
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
Conversation
Closes #6890
As described in the issue, sometimes 64-bit (long long) variables used for timestamps were being put through the INTEGER() macro which would fail in the tryto() routine. This PR changes the fields that need to hold these timestamps to be BIGINT.
Before changing to Ready for Review, I have a question:
Should the type actually be DATETIME, as is already the case for the certificates table spec? I've been told the data type is actually ignored and everything ends up text before being stored in the table, but, definitely the INTEGER casting function was failing out and needed to be changed.
Second question: will this affect 32-bit builds? I assume not, since processes already used BIGINT for its timestamps.
One other note: in logged_in_users.cpp for non-Windows systems (so, the POSIX implementation of the table), the code is taking the 64-bit timestamp from utmp and discarding the lower 32-bits (the microseconds). Is that desirable?
I wasn't able to get feedback on this, maybe someone could weigh in. My question is about what was intended to be, so we can adjust the data types macro calls to be consistent.
@mike-myers-tob I fear that there's not much consistency with it, although I personally believe we should follow what all sql servers (at least MySQL and SQL Server) normally show with that column type, which is text with the format YYYY-MM-DD HH:MI:SS.
Now the reason why I'm saying there's no consistency is because in sqlite while the DATETIME type exists, it exists only for compatibility reasons and it maps actually to the another type which is NUMERIC (https://www.sqlite.org/datatype3.html in the 3.1.1. Affinity Name Examples section).
Now we (osquery core) don't actually support DATETIME or NUMERIC as sqlite does, but we map DATETIME to TEXT
But it seems that we then expect it to be a timestamp:
query = "select CAST(seconds AS DATETIME) FROM time";
DATETIME should tell the user of the table what kind of format is expected.So as you've seen there's only one usage of that column type, in the certificates table and it should show dates not a timestamp as it's showing now.
Now to answer your question on what to put then, I would personally put BIGINT because a timestamp is a number.
Putting TEXT simply goes around any possible check we can do internally to see if the value is somehow incorrect.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
No one assigned
None yet
No milestone
Successfully merging this pull request may close these issues.
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK