Hi Danny, Danny Milosavljevic <dan...@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 <dan...@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’.