779310 - Remove string literals from slowSql reports

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Vladan Djeric (:vladan)]

Through bug 776469, we found that some extensions create prepared SQL statements out of strings containing unparameterized arguments. We should sanitize such slowSQL strings before reporting them to Telemetry

Comment 1

[Vladan Djeric (:vladan)]

Attachment: Sanitize slowSQL reports

Comment 2

[Vladan Djeric (:vladan)]

Attachment: Sanitize slowSQL reports

Comment 3

[Vladan Djeric (:vladan)]

Attachment: Sanitize slowSQL reports and re-enable slowSQL reporting

Comment 4

[(dormant account)]

Comment on attachment 648538 [details] [diff] [review]
Sanitize slowSQL reports and re-enable slowSQL reporting



+bool
+TelemetryImpl::SanitizeSQL(nsACString &sql) {

SanitizeSQL needs a big fat comment describing what it's doing, states used.

I'm not sure why you are looking for double quotes, those can only identifier names in sql(i think). 

Please do not dup into a C string...which you inplacemodify to make append easy. Instead scan the original string, provide a new string for output, use nsDependentSubstring to avoid allocating substrings. That will be less messy.
C strings lead to issues like leaks(where is your NS_free?).
I think our strings are infallible, this will mean that the method no longer needs to have a return value(ie it can't fail).

+  fullSQL.AppendPrintf(" /* %s */", dbName.BeginReading());
A bit messy that it's being done in all cases, but that seems ok.

Comment 5

[Vladan Djeric (:vladan)]

(In reply to Taras Glek (:taras) from comment #4)
> I'm not sure why you are looking for double quotes, those can only
> identifier names in sql(i think). 

Double quotes can also represent string literals. There was an exception made for compatibility: http://www.sqlite.org/lang_keywords.html

Comment 6

[Vladan Djeric (:vladan)]

Attachment: Sanitize slowSQL reports and re-enable slowSQL reporting, v2

Applied review

Comment 7

[(dormant account)]

Did you mean to ask for review?

Comment 8

[Vladan Djeric (:vladan)]

(In reply to Taras Glek (:taras) from comment #7)
> Did you mean to ask for review?

Yes. Should we wait until I add filtering for "IN" clauses containing a series of strings or numbers?

Comment 9

[(dormant account)]

(In reply to Vladan Djeric (:vladan) from comment #8)
> (In reply to Taras Glek (:taras) from comment #7)
> > Did you mean to ask for review?
> 
> Yes. Should we wait until I add filtering for "IN" clauses containing a
> series of strings or numbers?

no, can do that in a followup

Comment 10

[(dormant account)]

Comment on attachment 651050 [details] [diff] [review]
Sanitize slowSQL reports and re-enable slowSQL reporting, v2

Maybe Nathan has something clever to say about this.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 651050 [details] [diff] [review]
Sanitize slowSQL reports and re-enable slowSQL reporting, v2

Review of attachment 651050 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below changes.

::: toolkit/components/telemetry/Telemetry.cpp
@@ +109,5 @@
>    struct StmtStats {
> +    struct {
> +      uint32_t hitCount;
> +      uint32_t totalTime;
> +    } mainThread;

I'd suggest making this struct a named struct, so you can say:

struct StmtStats {
  struct stats mainThread;
  struct stats otherThreads;
};

which then enables other pieces of the code to become nicer, like ReflectSQL, below.

@@ +131,2 @@
>    static void StoreSlowSQL(const nsACString &offender, uint32_t delay,
> +                           bool isSanitized);

I'd suggest adding:

enum SanitizedState { Sanitized, Unsanitized };

and making this function take an |enum SanitizedState| instead of a bool, with appropriate == checking.  (Been reading about how bools in APIs virtually always want to be better-named enum parameters.)

@@ +135,5 @@
> +                                   JSObject *obj);
> +  static bool ReflectOtherThreadsSQL(SlowSQLEntryType *entry, JSContext *cx,
> +                                    JSObject *obj);
> +  static bool ReflectSQL(SlowSQLEntryType *entry, bool mainThread, JSContext *cx,
> +                         JSObject *obj);

With the suggested named struct change, this function can just take a struct stats* pointer, appropriately selected by its caller.  No more mainThread parameter.  The checking for ->hitCount == 0 can happen here, in one place, too.

Comment 12

[Vladan Djeric (:vladan)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8d115b3b7e1

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a8d115b3b7e1

Comment 14

[Vladan Djeric (:vladan)]

Should we uplift this patch to re-enable slowSQL reporting? Maybe after it has had some time to soak?

Comment 15

[(dormant account)]

(In reply to Vladan Djeric (:vladan) from comment #14)
> Should we uplift this patch to re-enable slowSQL reporting? Maybe after it
> has had some time to soak?

I would rather not. Nightlies will give us enough slowsql data. Would rather not uplift possible string-parsing bugs.