Re: Cuirass news

[ludo@gnu.org (Ludovic Courtès)]

Hi Danny,

Danny Milosavljevic <dannym@scratchpost.org> skribis:

> I saw that (cuirass database) has some problems with sql injection.
> I defused it a little, see attached patch.

Yes, I was unhappy with that, glad you fixed it.  :-)

> While we are at it, we can also reuse prepared statements (using the
> sqltext as key to find the right one).

Indeed, good idea!

> I also monitor sqlite accesses now - maybe that's overkill (see "with-mutex").

We don’t need mutexes: a given <db> is only ever used from one thread at
a time.  Sometimes we have several threads accessing the database, but
they do so through different handlers, which SQLite handles correctly.

Some comments:

> From b8fdd9c4e3a11f11c8d948ee07b2003fa4981f81 Mon Sep 17 00:00:00 2001
> From: Danny Milosavljevic <dannym@scratchpost.org>
> Date: Fri, 26 Jan 2018 15:16:04 +0100
> Subject: [PATCH] database: Make 'sqlite-exec' reuse the prepared statement.
> Tags: patch
>
> * src/cuirass/database.scm (%sqlite-exec): Delete variable.
> (<db>): New variable.
> (%wrap-db): New variable.
> (%sqlite-prepare): New variable.
> (%sqlite-bind-args): New variable.
> (%sqlite-fetch-all): New variable.
> (sqlite-exec): Modify.
> (db-init): Use %wrap-db.
> (db-open): Use %wrap-db.
> (db-close): Modify.
> (db-add-specification): Adjust for prepared statement parameters.
> (db-get-specifications): Adjust for prepared statement parameters.
> (db-add-derivation): Adjust for prepared statement parameters.
> (db-get-derivation): Adjust for prepared statement parameters.
> (db-add-evaluation): Adjust for prepared statement parameters.
> (db-add-build): Adjust for prepared statement parameters.
> (db-update-build-status!): Adjust for prepared statement parameters.
> (db-get-build): Adjust for prepared statement parameters.
> (db-get-builds): Adjust for prepared statement parameters.
> (db-get-stamp): Adjust for prepared statement parameters.
> (db-add-stamp): Adjust for prepared statement parameters.

[...]

> +(define (%wrap-db native-db)
> +  (db native-db (make-mutex) (make-weak-key-hash-table)))
> +
> +(define (%sqlite-prepare db sqlsym sqltext)
> +  (with-mutex (db-lock db)
> +    (let ((stmt (sqlite-prepare (db-native-db db) sqltext)))
> +      (hashq-set! (db-stmts db) sqlsym stmt)
> +      stmt)))

I’m not sure what ‘sqlsym’ is.  Apparently it’s a symbol derived from
the SQL statement, right?  I don’t think it’s necessary.

Instead you can simply make that hash table a regular (non-weak) hash
table that maps strings (SQL text) to prepared statements.  You’d also
need to use ‘hash-set!’ and ‘hash-ref’ instead of ‘hashq-set!’ and
‘hash-ref’ since strings should be compared with ‘equal?’, not ‘eq?’.

However, could the hash table grow indefinitely if there are always new
statements prepared?

> +(define (%sqlite-bind-args stmt args)
> +  (let ((argsi (zip (iota (length args)) args)))
> +    (for-each (match-lambda ((i arg)
> +                (sqlite-bind stmt (1+ i) arg)))
> +              argsi)))

You can make it (note the indentation of ‘match-lambda’):

  (for-each (match-lambda
              ((i arg)
               (sqlite-bind stmt (1+ i) arg)))
            (iota (length args))
            args)

>  (define-syntax sqlite-exec
> -  ;; Note: Making it a macro so -Wformat can do its job.
>    (lambda (s)
> -    "Wrap 'sqlite-prepare', 'sqlite-step', and 'sqlite-finalize'.  Send to given
> -SQL statement to DB.  FMT and ARGS are passed to 'format'."
>      (syntax-case s ()
> -      ((_ db fmt args ...)
> -       #'(%sqlite-exec db (format #f fmt args ...)))
> -      (id
> -       (identifier? #'id)
> -       #'(lambda (db fmt . args)
> -           (%sqlite-exec db (apply format #f fmt args)))))))
> +     ((_ db sqltext arg ...) (string? (syntax->datum #'sqltext))
> +      #`(let* ((sqlsym (quote #,(datum->syntax #'here (string->symbol (string-trim (syntax->datum #'sqltext))))))
> +               (stmt (or (hashq-ref (db-stmts db) sqlsym)
> +                         (%sqlite-prepare db sqlsym sqltext))))
> +          (with-mutex (db-lock db)
> +            (%sqlite-bind-args stmt (list arg ...))
> +            (%sqlite-fetch-all stmt))))

I think we can turn ‘sqlite-exec’ back into a procedure.  The only
reason to make it a macro was to have -Wformat support, as noted in the
comment.

Otherwise LGTM.

Could you prepare an updated patch to address these and to remove the
mutex?

Thank you!

Ludo’.