From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: X-Spam-Status: No, score=-4.0 required=3.0 tests=ALL_TRUSTED,AWL,BAYES_00, T_SCC_BODY_TEXT_LINE shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from localhost (dcvr.yhbt.net [127.0.0.1]) by dcvr.yhbt.net (Postfix) with ESMTP id 321571F9FC; Mon, 14 Feb 2022 05:37:26 +0000 (UTC) From: Eric Wong To: meta@public-inbox.org Subject: [PATCH 2/2] sharedkv: avoid ambiguity for numeric-like string keys Date: Mon, 14 Feb 2022 05:37:25 +0000 Message-Id: <20220214053725.1080495-3-e@80x24.org> In-Reply-To: <20220214053725.1080495-1-e@80x24.org> References: <20220214053725.1080495-1-e@80x24.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit List-Id: While we only store URLs and binary SHA-1/SHA-256 values in skv at the moment, we may store potentially ambiguous keys/values in the future. It's possible to store "02" and have it treated as `2' unless explicitly binding parameters as SQL_BLOB. This behavior was independent of the sqlite_unicode parameter as evidenced by the new tests. I only noticed this bug while hacking on another project using DBD::SQLite, and not while hacking on public-inbox itself. --- lib/PublicInbox/SharedKV.pm | 30 +++++++++++++++++++++++------- t/shared_kv.t | 3 +++ 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/lib/PublicInbox/SharedKV.pm b/lib/PublicInbox/SharedKV.pm index d49a39c1..90ccf2b4 100644 --- a/lib/PublicInbox/SharedKV.pm +++ b/lib/PublicInbox/SharedKV.pm @@ -9,7 +9,7 @@ use strict; use v5.10.1; use parent qw(PublicInbox::Lock); use File::Temp qw(tempdir); -use DBI (); +use DBI qw(:sql_types); # SQL_BLOB use PublicInbox::Spawn; use File::Path qw(rmtree make_path); @@ -59,9 +59,12 @@ sub new { sub set_maybe { my ($self, $key, $val, $lock) = @_; $lock //= $self->lock_for_scope_fast; - my $e = $self->{dbh}->prepare_cached(<<'')->execute($key, $val); + my $sth = $self->{dbh}->prepare_cached(<<''); INSERT OR IGNORE INTO kv (k,v) VALUES (?, ?) + $sth->bind_param(1, $key, SQL_BLOB); + $sth->bind_param(2, $val, SQL_BLOB); + my $e = $sth->execute; $e == 0 ? undef : $e; } @@ -88,20 +91,30 @@ sub keys { } else { @pfx = (); # [0] may've been undef } - map { $_->[0] } @{$self->dbh->selectall_arrayref($sql, undef, @pfx)}; + my $sth = $self->dbh->prepare($sql); + if (@pfx) { + $sth->bind_param(1, $pfx[0], SQL_BLOB); + $sth->bind_param(2, $pfx[1]); + } + $sth->execute; + map { $_->[0] } @{$sth->fetchall_arrayref}; } sub set { my ($self, $key, $val) = @_; if (defined $val) { - my $e = $self->{dbh}->prepare_cached(<<'')->execute($key, $val); + my $sth = $self->{dbh}->prepare_cached(<<''); INSERT OR REPLACE INTO kv (k,v) VALUES (?,?) + $sth->bind_param(1, $key, SQL_BLOB); + $sth->bind_param(2, $val, SQL_BLOB); + my $e = $sth->execute; $e == 0 ? undef : $e; } else { - $self->{dbh}->prepare_cached(<<'')->execute($key); + my $sth = $self->{dbh}->prepare_cached(<<''); DELETE FROM kv WHERE k = ? + $sth->bind_param(1, $key, SQL_BLOB); } } @@ -110,7 +123,8 @@ sub get { my $sth = $self->{dbh}->prepare_cached(<<'', undef, 1); SELECT v FROM kv WHERE k = ? - $sth->execute($key); + $sth->bind_param(1, $key, SQL_BLOB); + $sth->execute; $sth->fetchrow_array; } @@ -121,9 +135,11 @@ sub xchg { if (defined $newval) { set($self, $key, $newval); } else { - $self->{dbh}->prepare_cached(<<'')->execute($key); + my $sth = $self->{dbh}->prepare_cached(<<''); DELETE FROM kv WHERE k = ? + $sth->bind_param(1, $key, SQL_BLOB); + $sth->execute; } $oldval; } diff --git a/t/shared_kv.t b/t/shared_kv.t index 8b4f9c29..8dfd3b25 100644 --- a/t/shared_kv.t +++ b/t/shared_kv.t @@ -42,5 +42,8 @@ undef $skv; ok(!-d $skv_tmpdir, 'temporary dir gone'); $skv = PublicInbox::SharedKV->new("$tmpdir/dir", 'base'); ok(-e "$tmpdir/dir/base.sqlite3", 'file created'); +$skv->dbh; +ok($skv->set_maybe('02', '2'), "`02' set"); +ok($skv->set_maybe('2', '2'), "`2' set (no match on `02')"); done_testing;