From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42716) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ek6g0-0004bn-Ov for guix-patches@gnu.org; Fri, 09 Feb 2018 06:17:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ek6fu-0004Ru-Cb for guix-patches@gnu.org; Fri, 09 Feb 2018 06:17:08 -0500 Received: from debbugs.gnu.org ([208.118.235.43]:55071) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1ek6fu-0004Rk-9T for guix-patches@gnu.org; Fri, 09 Feb 2018 06:17:02 -0500 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1ek6ft-0005NS-Ul for guix-patches@gnu.org; Fri, 09 Feb 2018 06:17:02 -0500 Subject: [bug#30386] [PATCH v2 cuirass] database: Prevent SQL injection. Resent-Message-ID: Date: Fri, 9 Feb 2018 12:16:11 +0100 From: Danny Milosavljevic Message-ID: <20180209121612.09a0cf53@scratchpost.org> In-Reply-To: <87r2pu4hk4.fsf@gnu.org> References: <20180207231258.31169-1-dannym@scratchpost.org> <20180208163432.9468-1-dannym@scratchpost.org> <87r2pu4hk4.fsf@gnu.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+kyle=kyleam.com@gnu.org Sender: "Guix-patches" To: Ludovic =?UTF-8?Q?Court=C3=A8s?= Cc: 30386@debbugs.gnu.org Hi Ludo, no worries! > optimization looks good, provided the extra conditions don=E2=80=99t make= sqlite > slower. =20 Compared to parsing the SQL text again and again (which is dead slow), I th= ink an extra NULL check *on the same field* is not going to matter at all. Even compared to using lots of main memory and thus not being able to use the processor's cache (if we had lots of prepared statements), I think an extra NULL check is still better :) Of course once we have a lot of data in the tables, the actual lookup costs will dwarf any setup costs. Then still, it's checking the same field that's used anyway, so the extra cost should be neglible. >Do you think we can salvage this bit from your patch? =20 Sure. > It might allow us to use =E2=80=98sqlite-exec=E2=80=99 directly, and thus > benefit from the binding support in there, as in: >=20 > (sqlite-exec db "=E2=80=A6 WHERE " id " is NULL or =E2=80=A6") I added sqlite-bind-arguments with keyword arguments specifically so sqlite= -exec doesn't suck. So it would be like (sqlite-exec db "SELECT =E2=80=A6 :a =E2=80=A6 :b =E2= =80=A6 :a" #:a 42 #:b 2) Before, it was: (sqlite-exec db "SELECT =E2=80=A6 ? =E2=80=A6 ? =E2=80=A6 ?" 42 2 42) which repeated stuff - and was very fragile when changing things (one can e= asily get the order wrong and it would not have errored out).