Closed
Bug 303688
Opened 19 years ago
Closed 18 years ago
Eliminate deprecated Bugzilla::DB routines from attachment.cgi
Categories
(Bugzilla :: Bugzilla-General, enhancement)
Tracking
()
RESOLVED
FIXED
Bugzilla 3.0
People
(Reporter: wicked, Assigned: gregaryh)
References
Details
Attachments
(1 file, 3 obsolete files)
25.48 KB,
patch
|
LpSolit
:
review+
|
Details | Diff | Splinter Review |
These lines need to rewritten to use DBI: attachment.cgi:153: SendSQL("SELECT bug_id, isprivate FROM attachments WHERE attach_id = $attach_id"); attachment.cgi:154: MoreSQLData() attachment.cgi:158: (my $bugid, my $isprivate) = FetchSQLData(); attachment.cgi:206: SendSQL("SELECT attach_id FROM attachments WHERE " . attachment.cgi:209: FetchSQLData() attachment.cgi:217: SendSQL("SELECT product_id attachment.cgi:222: my $productid = FetchOneColumn(); attachment.cgi:231: SendSQL("SELECT product_id attachment.cgi:234: my $productid = FetchOneColumn(); attachment.cgi:403: SendSQL("SELECT bug_id, isobsolete, description attachment.cgi:407: MoreSQLData() attachment.cgi:410: my ($bugid, $isobsolete, $description) = FetchSQLData(); attachment.cgi:477: SendSQL("SELECT mimetype, filename, thedata FROM attachments " . attachment.cgi:479: my ($contenttype, $filename, $thedata) = FetchSQLData(); attachment.cgi:601: SendSQL("SELECT bug_id, description, ispatch, thedata FROM attachments WHERE attach_id = $id"); attachment.cgi:602: my ($bugid, $description, $ispatch, $thedata) = FetchSQLData(); attachment.cgi:731: SendSQL("SELECT bug_id, description, ispatch, thedata FROM attachments " . attachment.cgi:733: my ($bugid, $description, $ispatch, $thedata) = FetchSQLData(); attachment.cgi:761: SendSQL("SELECT attach_id, description FROM attachments WHERE bug_id = $bugid AND ispatch = 1 ORDER BY creation_ts DESC"); attachment.cgi:763: while (my ($other_id, $other_desc) = FetchSQLData()) { attachment.cgi:800: SendSQL("SELECT attach_id, " . attachment.cgi:807: while (MoreSQLData()) attachment.cgi:812: $a{'datasize'}) = FetchSQLData(); attachment.cgi:822: SendSQL("SELECT short_desc, assigned_to FROM bugs " . attachment.cgi:824: my ($bugsummary, $assignee_id) = FetchSQLData(); attachment.cgi:854: SendSQL("SELECT attach_id, description, isprivate attachment.cgi:860: while ( MoreSQLData() ) { attachment.cgi:862: ($a{'id'}, $a{'description'}, $a{'isprivate'}) = FetchSQLData(); attachment.cgi:869: SendSQL("SELECT short_desc, assigned_to FROM bugs attachment.cgi:871: my ($bugsummary, $assignee_id) = FetchSQLData(); attachment.cgi:880: SendSQL("SELECT product_id, component_id FROM bugs attachment.cgi:882: my ($product_id, $component_id) = FetchSQLData(); attachment.cgi:941: my $sql_filename = SqlQuote($filename); attachment.cgi:942: my $description = SqlQuote($cgi->param('description')); attachment.cgi:943: my $contenttype = SqlQuote($cgi->param('contenttype')); attachment.cgi:948: my $sql_timestamp = SqlQuote($timestamp); attachment.cgi:1008: SendSQL("UPDATE attachments SET isobsolete = 1 " . attachment.cgi:1010: SendSQL("INSERT INTO bugs_activity (bug_id, attach_id, who, bug_when, attachment.cgi:1024: SendSQL("SELECT " . join(", ", @fields) . " " . attachment.cgi:1030: my @oldvalues = FetchSQLData(); attachment.cgi:1036: @oldvalues = map(SqlQuote($_), @oldvalues); attachment.cgi:1037: @newvalues = map(SqlQuote($_), @newvalues); attachment.cgi:1040: SendSQL("UPDATE bugs SET delta_ts = $sql_timestamp, " . attachment.cgi:1052: SendSQL("INSERT INTO bugs_activity " . attachment.cgi:1090: SendSQL("SELECT description, mimetype, filename, bug_id, ispatch, isobsolete, isprivate, LENGTH(thedata) attachment.cgi:1092: my ($description, $contenttype, $filename, $bugid, $ispatch, $isobsolete, $isprivate, $datasize) = FetchSQLData(); attachment.cgi:1098: SendSQL("SELECT attach_id FROM attachments WHERE bug_id = $bugid ORDER BY attach_id"); attachment.cgi:1100: push(@bugattachments, FetchSQLData()) while (MoreSQLData()); attachment.cgi:1101: SendSQL("SELECT short_desc FROM bugs WHERE bug_id = $bugid"); attachment.cgi:1102: my ($bugsummary) = FetchSQLData(); attachment.cgi:1105: SendSQL("SELECT product_id, component_id FROM bugs WHERE bug_id = $bugid"); attachment.cgi:1106: my ($product_id, $component_id) = FetchSQLData(); attachment.cgi:1192: SendSQL("SELECT description, mimetype, filename, ispatch, isobsolete, isprivate attachment.cgi:1195: $oldisobsolete, $oldisprivate) = FetchSQLData(); attachment.cgi:1198: my $quoteddescription = SqlQuote($cgi->param('description')); attachment.cgi:1199: my $quotedcontenttype = SqlQuote($cgi->param('contenttype')); attachment.cgi:1200: my $quotedfilename = SqlQuote($cgi->param('filename')); attachment.cgi:1203: SendSQL("SELECT NOW()"); attachment.cgi:1204: my $timestamp = FetchOneColumn(); attachment.cgi:1214: SendSQL("UPDATE attachments attachment.cgi:1225: my $sql_timestamp = SqlQuote($timestamp); attachment.cgi:1227: my $quotedolddescription = SqlQuote($olddescription); attachment.cgi:1229: SendSQL("INSERT INTO bugs_activity (bug_id, attach_id, who, bug_when, attachment.cgi:1235: my $quotedoldcontenttype = SqlQuote($oldcontenttype); attachment.cgi:1237: SendSQL("INSERT INTO bugs_activity (bug_id, attach_id, who, bug_when, attachment.cgi:1243: my $quotedoldfilename = SqlQuote($oldfilename); attachment.cgi:1245: SendSQL("INSERT INTO bugs_activity (bug_id, attach_id, who, bug_when, attachment.cgi:1252: SendSQL("INSERT INTO bugs_activity (bug_id, attach_id, who, bug_when, attachment.cgi:1259: SendSQL("INSERT INTO bugs_activity (bug_id, attach_id, who, bug_when, attachment.cgi:1266: SendSQL("INSERT INTO bugs_activity (bug_id, attach_id, who, bug_when,
Updated•19 years ago
|
Assignee: general → LpSolit
Target Milestone: --- → Bugzilla 2.22
Updated•19 years ago
|
Target Milestone: Bugzilla 2.22 → Bugzilla 2.24
Assignee | ||
Updated•18 years ago
|
Assignee: LpSolit → ghendricks
Assignee | ||
Comment 1•18 years ago
|
||
Attachment #213757 -
Flags: review?(mkanat)
Comment 2•18 years ago
|
||
Comment on attachment 213757 [details] [diff] [review] Patch v1 >@@ -734,10 +745,12 @@ > my $context = validateContext(); > > # Get patch data >- SendSQL("SELECT bug_id, description, ispatch, thedata FROM attachments " . >+ my $sth = $dbh->prepare( >+ "SELECT bug_id, description, ispatch, thedata FROM attachments " . > "INNER JOIN attach_data ON id = attach_id " . >- "WHERE attach_id = $attach_id"); >- my ($bugid, $description, $ispatch, $thedata) = FetchSQLData(); >+ "WHERE attach_id = ?"); >+ $sth->execute($attach_id); >+ my ($bugid, $description, $ispatch, $thedata) = $sth->fetchrow_array; Nit: Why don't you just do this with selectrow_array like you do everywhere else? >+ $sth = $dbh->prepare("SELECT attach_id, description >+ FROM attachments >+ WHERE bug_id = ? >+ AND ispatch = 1 >+ ORDER BY creation_ts DESC"); Nit: And same here. >@@ -803,22 +821,23 @@ >+ my $sth = $dbh->prepare_cached("SELECT attach_id, " . Don't use prepare_cached, it's not mod_perl safe for us. > $dbh->sql_date_format('creation_ts', '%Y.%m.%d %H:%i') . ", > mimetype, description, ispatch, isobsolete, isprivate, > LENGTH(thedata) > FROM attachments > INNER JOIN attach_data > ON attach_id = id >- WHERE bug_id = $bugid $privacy >+ WHERE bug_id = ? $privacy > ORDER BY attach_id"); >+ $sth->execute($bugid); > my @attachments; # the attachments array >- while (MoreSQLData()) >+ while (my @row = $sth->fetchrow_array) > { > my %a; # the attachment hash > ($a{'attachid'}, $a{'date'}, $a{'contenttype'}, > $a{'description'}, $a{'ispatch'}, $a{'isobsolete'}, $a{'isprivate'}, >- $a{'datasize'}) = FetchSQLData(); >+ $a{'datasize'}) = @row; You can just create all of this with fetchrow_hashref, if you use "AS" to name the columns properly. >@@ -828,9 +847,9 @@ >+ my ($bugsummary, $assignee_id) = $dbh->selectrow_array( >+ "SELECT short_desc, assigned_to FROM bugs " . >+ "WHERE bug_id = ?", undef, $bugid); This is good, by the way -- much better than creating a temporary @row variable. >@@ -859,24 +878,26 @@ >+ my $sth = $dbh->prepare_cached( prepare_cached, again. >+ while ( my @row = $sth->fetchrow_array ) { > my %a; # the attachment hash >- ($a{'id'}, $a{'description'}, $a{'isprivate'}) = FetchSQLData(); >+ ($a{'id'}, $a{'description'}, $a{'isprivate'}) = @row; And another time that you could use fetchrow_hashref. >@@ -904,7 +926,6 @@ > # Insert a new attachment into the database. > sub insert > { >- my $dbh = Bugzilla->dbh; Why'd you do this? subs *should* have their own $dbh. >@@ -963,22 +986,21 @@ >+ my ($sql_timestamp) = Bugzilla->dbh->selectrow_array("SELECT NOW()"); >+ trick_taint($sql_timestamp); You don't need to trick_taint that, it came out of the database, so it's not tainted. >+ VALUES (?,?,?,?,?,?,?,?,?)"); >+ $sth->execute($bugid, $sql_timestamp, $filename, >+ $description, $contenttype, $cgi->param('ispatch'), >+ $isurl, $isprivate, $userid); Does that work? $cgi->param('ispatch') is almost definitely tainted. It looks like it worked before, but... oh, I suppose we called detaint_natural on it earlier, yes? >- @oldvalues = map(SqlQuote($_), @oldvalues); >- @newvalues = map(SqlQuote($_), @newvalues); >+ @oldvalues = map($dbh->quote($_), @oldvalues); >+ @newvalues = map($dbh->quote($_), @newvalues); SqlQuote also detaints things, but $dbh->quote doesn't. > # Update the bug record. Note that this doesn't involve login_name. >- SendSQL("UPDATE bugs SET delta_ts = $sql_timestamp, " . >+ $dbh->do("UPDATE bugs SET delta_ts = $sql_timestamp, " . Was $sql_timestamp already sql-quoted? It shouldn't be; we should just pass it in through a placeholder. >+ $dbh->do("UPDATE attachments >+ SET description = ?, >+ mimetype = ?, >+ filename = ?, >+ ispatch = ?, >+ isobsolete = ?, >+ isprivate = ? >+ WHERE attach_id = ?", >+ undef, $description, $contenttype, $filename, >+ $cgi->param('ispatch'), $cgi->param('isobsolete'), >+ $cgi->param('isprivate'), $attach_id); Are all those $cgi->param things untainted?? >- VALUES ($bugid, $attach_id, $userid, $sql_timestamp, $fieldid, >- $oldispatch, " . $cgi->param('ispatch') . ")"); >+ VALUES (?,?,?,?,?,?,?)", >+ undef, $bugid, $attach_id, $userid, $sql_timestamp, $fieldid, >+ $oldispatch, $cgi->param('ispatch')); And here, too?
Attachment #213757 -
Flags: review?(mkanat) → review-
Assignee | ||
Comment 3•18 years ago
|
||
(In reply to comment #2) > > $dbh->sql_date_format('creation_ts', '%Y.%m.%d %H:%i') . ", > > mimetype, description, ispatch, isobsolete, isprivate, > > LENGTH(thedata) > > FROM attachments > > INNER JOIN attach_data > > ON attach_id = id > >- WHERE bug_id = $bugid $privacy > >+ WHERE bug_id = ? $privacy > > ORDER BY attach_id"); > >+ $sth->execute($bugid); > > my @attachments; # the attachments array > >- while (MoreSQLData()) > >+ while (my @row = $sth->fetchrow_array) > > { > > my %a; # the attachment hash > > ($a{'attachid'}, $a{'date'}, $a{'contenttype'}, > > $a{'description'}, $a{'ispatch'}, $a{'isobsolete'}, $a{'isprivate'}, > >- $a{'datasize'}) = FetchSQLData(); > >+ $a{'datasize'}) = @row; > > You can just create all of this with fetchrow_hashref, if you use "AS" to > name the columns properly. Oh but that is so much more work and I am LAZY! But for you? sure ;-) > >@@ -963,22 +986,21 @@ > >+ my ($sql_timestamp) = Bugzilla->dbh->selectrow_array("SELECT NOW()"); > >+ trick_taint($sql_timestamp); > > You don't need to trick_taint that, it came out of the database, so it's not > tainted. > > >+ VALUES (?,?,?,?,?,?,?,?,?)"); > >+ $sth->execute($bugid, $sql_timestamp, $filename, > >+ $description, $contenttype, $cgi->param('ispatch'), > >+ $isurl, $isprivate, $userid); > > Does that work? $cgi->param('ispatch') is almost definitely tainted. It looks > like it worked before, but... oh, I suppose we called detaint_natural on it > earlier, yes? That is all handled in validateIsPatch > > >- @oldvalues = map(SqlQuote($_), @oldvalues); > >- @newvalues = map(SqlQuote($_), @newvalues); > >+ @oldvalues = map($dbh->quote($_), @oldvalues); > >+ @newvalues = map($dbh->quote($_), @newvalues); > > SqlQuote also detaints things, but $dbh->quote doesn't. Yes, I am aware, that is why I have added a bunch of trick_taints in this patch ;-) However, in this case @oldvalues is all out of the DB (and as per your comment above we don't need to worry about those) and @newvalues are @newvalues = ($userid, "ASSIGNED", "", 1, Bugzilla->user->login) of which only $userid would be questionable but as we see further up it is Bugzilla->user->id. So I don't think we need to worry about them. > >+ $dbh->do("UPDATE attachments > >+ SET description = ?, > >+ mimetype = ?, > >+ filename = ?, > >+ ispatch = ?, > >+ isobsolete = ?, > >+ isprivate = ? > >+ WHERE attach_id = ?", > >+ undef, $description, $contenttype, $filename, > >+ $cgi->param('ispatch'), $cgi->param('isobsolete'), > >+ $cgi->param('isprivate'), $attach_id); > > Are all those $cgi->param things untainted?? > > >- VALUES ($bugid, $attach_id, $userid, $sql_timestamp, $fieldid, > >- $oldispatch, " . $cgi->param('ispatch') . ")"); > >+ VALUES (?,?,?,?,?,?,?)", > >+ undef, $bugid, $attach_id, $userid, $sql_timestamp, $fieldid, > >+ $oldispatch, $cgi->param('ispatch')); > > And here, too? > Yes, in thier appropriate subroutines.
Attachment #213757 -
Attachment is obsolete: true
Attachment #213820 -
Flags: review?(mkanat)
Comment 4•18 years ago
|
||
Comment on attachment 213820 [details] [diff] [review] Patch v2 All routines which use $dbh should define $dbh in the routines themselves. Else if we move these routines elsewhere, we will probably break them because $dbh will be undefined. >@@ -151,12 +151,15 @@ >+ my @row = $dbh->selectrow_array("SELECT bug_id, isprivate >+ FROM attachments >+ WHERE attach_id = ?", >+ undef, $attach_id); >+ ThrowUserError("invalid_attach_id", { attach_id => $attach_id }) >+ unless @row; > > # Make sure the user is authorized to access this attachment's bug. >+ (my $bugid, my $isprivate) = @row; Much better is to write: my (bugid, $isprivate) = $dbh->selectrow_array(...); and get rid of @row. >@@ -204,23 +207,27 @@ >+ my $ref = $dbh->selectrow_arrayref("SELECT attach_id FROM attachments >+ WHERE attach_id = ? >+ AND submitter_id = ?", >+ undef, $attach_id, Bugzilla->user->id); We have never used selectrow_arrayref(), which doesn't make sense here. We want the attachment ID, so use selectrow_array(). Moreover, as soon as you have more than one parameters, use parens to group these parameters => "undef, ($attach_id, Bugzilla->user->id)". >+ ThrowUserError("illegal_attachment_edit", >+ { attach_id => $attach_id }) >+ unless $ref; This is arbitrary indentation... Write: $ref || ThrowUserError("illegal_attachment_edit", { attach_id => $attach_id }); We have enough place to write it on one line. >@@ -401,14 +409,13 @@ >+ my @row = $dbh->selectrow_array("SELECT bug_id, isobsolete, description >+ FROM attachments WHERE attach_id = ?", undef, $attachid); > > # Make sure the attachment exists in the database. >+ ThrowUserError("invalid_attach_id", $vars) unless @row; > >+ my ($bugid, $isobsolete, $description) = @row; Here again, get rid of @row, as above. >@@ -475,10 +482,12 @@ >+ my $sth = $dbh->prepare( >+ "SELECT mimetype, filename, thedata FROM attachments " . > "INNER JOIN attach_data ON id = attach_id " . >+ "WHERE attach_id = ?"); >+ $sth->execute($attach_id); >+ my ($contenttype, $filename, $thedata) = $sth->fetchrow_array; Use selectrow_array() instead of prepare(), execute(), fetchrow_array(). >@@ -600,12 +609,14 @@ >+ my $sth = $dbh->prepare( >+ "SELECT bug_id, description, ispatch, thedata " . > "FROM attachments " . > "INNER JOIN attach_data " . > "ON id = attach_id " . >+ "WHERE attach_id = ?"); >+ $sth->execute($id); >+ my ($bugid, $description, $ispatch, $thedata) = $sth->fetchrow_array; Use selectrow_array() here too. >@@ -764,9 +775,14 @@ >+ my $sth = $dbh->prepare("SELECT attach_id, description >+ FROM attachments >+ WHERE bug_id = ? >+ AND ispatch = 1 >+ ORDER BY creation_ts DESC"); Please align the SQL query vertically. This applies at some other places too. >+ $sth->execute($bugid); > my $select_next_patch = 0; >+ while (my ($other_id, $other_desc) = $sth->fetchrow_array) { Moreover, use selectall_arrayref() and then do a loop over the array you get. >@@ -803,34 +819,32 @@ >+ my $sth = $dbh->prepare("SELECT attach_id AS attachid, " . >+ $sth->execute($bugid); > my @attachments; # the attachments array >+ while (my $a = $sth->fetchrow_hashref) You should use selectall_arrayref() combined with {'Slice' => {}} instead. >@@ -859,24 +873,24 @@ >+ my $sth = $dbh->prepare( >+ "SELECT attach_id AS id, description, isprivate >+ $sth->execute($bugid); > my @attachments; # the attachments array >+ while ( my $a = $sth->fetchrow_hashref ) { > > # Add the hash representing the attachment to the array of attachments. >+ push @attachments, $a; > } Same comment here. This way, you don't need to push() anything. The array is already built. >@@ -925,16 +939,18 @@ > $filename = validateFilename(); >+ trick_taint($filename); >+ $contenttype = $cgi->param('contenttype'); >+ trick_taint($contenttype); In both cases, justify why you detaint these values. >@@ -963,22 +979,21 @@ >+ my $description = $cgi->param('description'); >+ trick_taint($description); Same comment here. >+ my ($sql_timestamp) = Bugzilla->dbh->selectrow_array("SELECT NOW()"); There is no reason to call it $sql_something, which were used when using SqlQuote(). It's a timestamp, call it $timestamp. > my $sth = $dbh->prepare("INSERT INTO attachments > (bug_id, creation_ts, filename, description, > mimetype, ispatch, isurl, isprivate, submitter_id) >+ VALUES (?,?,?,?,?,?,?,?,?)"); >+ $sth->execute($bugid, $sql_timestamp, $filename, >+ $description, $contenttype, $cgi->param('ispatch'), >+ $isurl, $isprivate, $userid); First of all, use $dbh->do(). Secondly, add a whitespace after commas, as we usually do. Third point: group parameters in parens (we are supposed to pass an array). These comments apply in several other places too. >@@ -1050,23 +1065,23 @@ >+ my @oldvalues = $dbh->selectrow_array( >+ "SELECT " . join(", ", @fields) . " " . > "FROM bugs " . > "INNER JOIN profiles " . > "ON profiles.userid = bugs.assigned_to " . >+ "WHERE bugs.bug_id = ?", undef, $bugid); I definitely don't understand the indentation you use with SQL queries. 8 whitespaces here. >+ $dbh->do("UPDATE bugs SET delta_ts = $sql_timestamp, " . > join(", ", map("$fields[$_] = $newvalues[$_]", (0..3))) . > " WHERE bug_id = $bugid"); Nit: You could as well use placeholders here too. >@@ -1201,83 +1217,93 @@ >+ my @row = $dbh->selectrow_array( >+ "SELECT description, mimetype, filename, ispatch, isobsolete, isprivate >+ FROM attachments WHERE attach_id = ?", undef, $attach_id); > my ($olddescription, $oldcontenttype, $oldfilename, $oldispatch, >+ $oldisobsolete, $oldisprivate) = @row; Get rid of @row. >+ my $description = $cgi->param('description'); >+ trick_taint($description); >+ my $contenttype = $cgi->param('contenttype'); >+ trick_taint($contenttype); >+ my $filename = $cgi->param('filename'); >+ trick_taint($filename); group all these trick_taint() below a comment explaining why you are detaining these values. >+ my ($sql_timestamp) = $dbh->selectrow_array("SELECT NOW()"); Same comment as before, don't write $sql_foo. >+ $dbh->do("UPDATE attachments >+ SET description = ?, >+ mimetype = ?, >+ filename = ?, >+ ispatch = ?, >+ isobsolete = ?, >+ isprivate = ? >+ WHERE attach_id = ?", >+ undef, $description, $contenttype, $filename, >+ $cgi->param('ispatch'), $cgi->param('isobsolete'), >+ $cgi->param('isprivate'), $attach_id); Parens. >+ $dbh->do("INSERT INTO bugs_activity (bug_id, attach_id, who, bug_when, > fieldid, removed, added) >+ VALUES (?,?,?,?,?,?,?)", >+ undef, $bugid, $attach_id, $userid, $sql_timestamp, $fieldid, >+ $olddescription, $description); As above, whitespaces after commas, and parens. This comment applies in other places too.
Attachment #213820 -
Flags: review?(mkanat) → review-
Assignee | ||
Comment 5•18 years ago
|
||
Attachment #213820 -
Attachment is obsolete: true
Attachment #216464 -
Flags: review?(mkanat)
Comment 6•18 years ago
|
||
Comment on attachment 216464 [details] [diff] [review] v3 > sub validateID > { >+ my $dbh = Bugzilla->dbh; > my $param = @_ ? $_[0] : 'id'; Nit: you should first get parameters passed to the routine @_ and then defined $dbh. That's the way we do it in the Bugzilla code. > sub validateCanEdit > { >+ my $dbh = Bugzilla->dbh; > my ($attach_id) = (@_); Same comment here and in other routines too. >+ my $sth = $dbh->prepare("SELECT attach_id, description >+ FROM attachments >+ WHERE bug_id = ? >+ AND ispatch = 1 >+ ORDER BY creation_ts DESC"); >+ $sth->execute($bugid); > my $select_next_patch = 0; >+ while (my ($other_id, $other_desc) = $sth->fetchrow_array) { Nit: you could use selectall_arrayref() and do a loop over the list of attachments being returned. >+ foreach my $a (@{$attachments}) Nit: you can write @$attachments. > # Add the hash representing the attachment to the array of attachments. >+ push @attachments, $a; No need to push $a. $a has already been updated. >+ Bugzilla::Flag::CancelRequests($bugid, $obsolete_id, $timestamp); Flag::update_activity(), called by Flag::CancelRequests(), requires a quoted timestamp (this will be fixed in another bug). So $dbh->quote($timestamp) is required here till Flag.pm is fixed. >+ $dbh->do("UPDATE bugs SET delta_ts = $timestamp, " . > join(", ", map("$fields[$_] = $newvalues[$_]", (0..3))) . >+ " WHERE bug_id = ?", undef, $bugid); Use a placeholder for the timestamp. > for (my $i = 0; $i < 4; $i++) { > if ($oldvalues[$i] ne $newvalues[$i]) { > my $fieldid = get_field_id($fields[$i]); >+ $dbh->do("INSERT INTO bugs_activity " . >+ "(bug_id, who, bug_when, fieldid, removed, added) " . >+ "VALUES (?,?,?,?,?,?)", >+ undef, $bugid, $userid, $timestamp, >+ $fieldid, $oldvalues[$i], $newvalues[$i]); > } > } Prepare the SQL query outside the FOR loop. Moreover, enclose the list of values in parens. >+ # we can trick taint until the cows come home thanks to placeholders Hum... s/trick taint/detaint/ and s/cows come home//. >+ # trick_taint is fun, even if your parents say otherwise. >+ # place holders are your friends >+ trick_taint($olddescription); Is it a test to make sure the reviewer reads the patch carefully? ;) Anyway, as $olddescription comes from the DB, it doesn't need to be detainted. >+ trick_taint($oldcontenttype); >+ trick_taint($oldfilename); Same comment here. All values coming from the DB do not need to be detainted.
Attachment #216464 -
Flags: review?(mkanat) → review-
Assignee | ||
Comment 7•18 years ago
|
||
(In reply to comment #6) > >+ my $sth = $dbh->prepare("SELECT attach_id, description > >+ FROM attachments > >+ WHERE bug_id = ? > >+ AND ispatch = 1 > >+ ORDER BY creation_ts DESC"); > >+ $sth->execute($bugid); > > my $select_next_patch = 0; > >+ while (my ($other_id, $other_desc) = $sth->fetchrow_array) { > > Nit: you could use selectall_arrayref() and do a loop over the list of > attachments being returned. This feels too invasive to me. Unless this is more efficient I would prefer to keep it simple. This way was the the least invasive. > > >+ foreach my $a (@{$attachments}) > > Nit: you can write @$attachments. > I know, but I like how the braces make it stand out. It helps me see it easier.
Attachment #216464 -
Attachment is obsolete: true
Attachment #218188 -
Flags: review?(LpSolit)
Comment 8•18 years ago
|
||
Comment on attachment 218188 [details] [diff] [review] V4 >+ my $sth = $dbh->do( Nit: $dbh->do() won't return anything useful. $sth is optional. The patch works correctly. r=LpSolit
Attachment #218188 -
Flags: review?(LpSolit) → review+
Updated•18 years ago
|
Status: NEW → ASSIGNED
Flags: approval?
Updated•18 years ago
|
Flags: approval? → approval+
Comment 9•18 years ago
|
||
Checking in attachment.cgi; /cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v <-- attachment.cgi new revision: 1.106; previous revision: 1.105 done
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•