Checksetup uses some bad SQL that is not cross-database compatible

RESOLVED FIXED in Bugzilla 2.20

Status

()

defect
P1
normal
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: mkanat, Assigned: mkanat)

Tracking

2.19.2
Bugzilla 2.20
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 1 obsolete attachment)

There are a few places that PostgreSQL chokes in checksetup code, before it can
even get to the bz_ functions in Bugzilla::DB. These need to be fixed before I
can fix each of those individual bz_ functions to be cross-database compatible.
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.20
These are simple fixes. I've verified that they work on Pg and on MySQL.
Attachment #176083 - Flags: review?(Tomas.Kopal)
Attachment #176083 - Flags: review?(bugzilla)
Comment on attachment 176083 [details] [diff] [review]
Fix bad quoting and insertion of "NULL" as a string

>-$sth = $dbh->prepare("SELECT description FROM products");
>+my $sth = $dbh->prepare("SELECT description FROM products");

this causes
"my" variable $sth masks earlier declaration in same scope at checksetup.pl
line 2386.


otherwise looks good.
Attachment #176083 - Flags: review?(bugzilla) → review-
(In reply to comment #2)
> this causes
> "my" variable $sth masks earlier declaration in same scope at checksetup.pl
> line 2386.

  Oh, that's part of the patch from bug 284172. :-) Oops. :-)
Comment on attachment 176083 [details] [diff] [review]
Fix bad quoting and insertion of "NULL" as a string

>Index: checksetup.pl
>@@ -2230,23 +1651,21 @@
> sub AddFDef ($$$) {
>     my ($name, $description, $mailhead) = (@_);
> 
>-    $name = $dbh->quote($name);
>-    $description = $dbh->quote($description);

Are you sure it's safe to leave this out? Can't the function be ever called
with user-supplied or otherwise unsafe data? (That's not suggestion, that's
really just question :-) ).

>@@ -2384,20 +1804,26 @@
> ###########################################################################
> # Create initial test product if there are no products present.
> ###########################################################################
>-$sth = $dbh->prepare("SELECT description FROM products");
>+my $sth = $dbh->prepare("SELECT description FROM products");

I remember this change was already in another patch :-).

>     $dbh->do("INSERT INTO fielddefs " .
...
>+    $dbh->do(q{INSERT INTO products(name, description, milestoneurl, 
...
>+    $dbh->do(qq{INSERT INTO versions (value, product_id) 

Can't we somehow make all the quoting more consistent? We are using single
quotes, double quotes, q{}, qq{} all over the code... But thats probably more
question for some general rules than for this patch...

Otherwise, it looks good...
Attachment #176083 - Flags: review?(bugzilla)
Attachment #176083 - Flags: review?(Tomas.Kopal)
Attachment #176083 - Flags: review-
Attachment #176083 - Flags: review+
> Are you sure it's safe to leave this out? Can't the function be ever called
> with user-supplied or otherwise unsafe data? (That's not suggestion, that's
> really just question :-) ).

it's save; using sql placeholders automatically performs the quoting.
Attachment #176083 - Flags: review?(bugzilla) → review-
Posted patch v2Splinter Review
OK, this one's all better. :-)
Attachment #176083 - Attachment is obsolete: true
Attachment #176226 - Flags: review?(bugzilla)
Comment on attachment 176226 [details] [diff] [review]
v2

r=glob
Attachment #176226 - Flags: review?(bugzilla) → review+
Flags: approval?
Flags: approval? → approval+
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.359; previous revision: 1.358
done
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.