From mboxrd@z Thu Jan 1 00:00:00 1970 From: ludo@gnu.org (Ludovic =?utf-8?Q?Court=C3=A8s?=) Subject: Re: Cuirass news Date: Sat, 27 Jan 2018 17:01:23 +0100 Message-ID: <87zi4z1eb0.fsf@gnu.org> References: <877es6x5xj.fsf@gnu.org> <87lggmjjgo.fsf@gmail.com> <87k1w6jjak.fsf@gmail.com> <87h8raxeym.fsf@gnu.org> <20180126153005.259a75e8@scratchpost.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:45056) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1efSv4-0003vP-Tv for guix-devel@gnu.org; Sat, 27 Jan 2018 11:01:31 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1efSv1-00063Y-TJ for guix-devel@gnu.org; Sat, 27 Jan 2018 11:01:31 -0500 Received: from hera.aquilenet.fr ([185.233.100.1]:57938) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1efSv1-00062T-Ng for guix-devel@gnu.org; Sat, 27 Jan 2018 11:01:27 -0500 In-Reply-To: <20180126153005.259a75e8@scratchpost.org> (Danny Milosavljevic's message of "Fri, 26 Jan 2018 15:30:05 +0100") List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: "Guix-devel" To: Danny Milosavljevic Cc: guix-devel Hi Danny, Danny Milosavljevic 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-mut= ex"). We don=E2=80=99t need mutexes: a given is only ever used from one thre= ad 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 > Date: Fri, 26 Jan 2018 15:16:04 +0100 > Subject: [PATCH] database: Make 'sqlite-exec' reuse the prepared statemen= t. > Tags: patch > > * src/cuirass/database.scm (%sqlite-exec): Delete variable. > (): 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=E2=80=99m not sure what =E2=80=98sqlsym=E2=80=99 is. Apparently it=E2=80= =99s a symbol derived from the SQL statement, right? I don=E2=80=99t think it=E2=80=99s necessary. Instead you can simply make that hash table a regular (non-weak) hash table that maps strings (SQL text) to prepared statements. You=E2=80=99d a= lso need to use =E2=80=98hash-set!=E2=80=99 and =E2=80=98hash-ref=E2=80=99 inst= ead of =E2=80=98hashq-set!=E2=80=99 and =E2=80=98hash-ref=E2=80=99 since strings should be compared with =E2=80=98e= qual?=E2=80=99, not =E2=80=98eq?=E2=80=99. 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 =E2=80=98match-lambda=E2=80=99): (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 (s= tring-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 =E2=80=98sqlite-exec=E2=80=99 back into a procedure. T= he 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=E2=80=99.