303088 - MoreSQLData and FetchSQLData/FetchOneColumn may return wrong results

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Frédéric Buclin]

DB::MoreSQLData checks if $_fetchahead is defined in order to know if the
previous query returned some results or not:

return 1 if defined $_fetchahead;

But only DB::FetchSQLData clears $_fetchahead after each call, meaning that
$_fetchahead may remain defined if you only check MoreSQLData without
effectively considering some results of the query.

Consider the following case:

SendSQL($query1);                      # returns more than one result
my $result1 = (MoreSQLData())? 1 : 0;  # $result1 = 1
SendSQL($query2);                      # returns no result (undef)
my $result2 = (FetchSQLData())? 1 : 0; # $result2 = 1 !!!

In this case, $result2 = 1 despite the fact that the second query returns no
results!!

This problem appears in editgroups.cgi when deleting a group, see ($action eq
"del").

Comment 1

[Frédéric Buclin]

Attachment: patch for all versions >= 2.17.4

SendSQL now undefines $_fetchahead.

Comment 2

[Marc Schumann [:Wurblzap]]

Comment on attachment 191334 [details] [diff] [review]
patch for all versions >= 2.17.4

r=wurblzap by basic spot-check testing on HEAD and thinking that it's the right
thing to do.

I think a second review from somebody who has an actual idea of possible
consequences would be good :)

Comment 3

[Dave Miller [:justdave]]

Comment on attachment 191334 [details] [diff] [review]
patch for all versions >= 2.17.4

This *looks* right to me, but I'm concerned about the mod_perl comment... 
bbaetz put that there, what's the consequences related to mod_perl here?

Comment 4

[Marc Schumann [:Wurblzap]]

Am I right in reading this as "Please make it clearer that the mod_perl comment
several lines before the first hunk spans down to $_fetchahead's new location"?

Comment 5

[Frédéric Buclin]

(In reply to comment #3)
> (From update of attachment 191334 [details] [diff] [review] [edit])
> This *looks* right to me, but I'm concerned about the mod_perl comment... 
> bbaetz put that there, what's the consequences related to mod_perl here?
> 

justdave, if you are only concerned by the mod_perl comment but not by the fix
itself, I can let this comment in place if you prefer; I don't care. I only
thought that having twice "XXX mod_perl" every 5 lines of code was useless, but
if you feel more confident with this comment added, I can do it.

Comment 6

[Bradley Baetz (:bbaetz)]

The mod_perl comment is just that the variable is unusable with mod_perl since
the previous results. The whole API is unusable due to this - mod_perl-safe
pages require the direct DBI calls. The idea was that everything in .pm files
(as opposed to globals.pl) was mod-perl safe, except where otherwise marked. But
then I never finished mod-perling bugzilla...

Re the old API, once a query has been made, it must be fully read. This is a
sucky API. See, eg, bug 189446 comment 2, bug 191085. I can't think what this
would break offhand, but you're touching code that dates back to the rev 1.1 of
globals.pl (although not back to globals.tcl)

The actual bug is the first SendSQL query. It probably broke when group
inheritance was added and that query could return more than one row (which
FetchOneColumn would have taken). ITs still wasteful; LIMIT 1 on the query does
what you want and is basically more efficient.

Here be dragons ;)

Comment 7

[Frédéric Buclin]

bbaetz, we haven't checked that this problem doesn't occur anywhere else in the
code, so fixing this particular place isn't enough to me. $_fetchahead really
has to be cleared when SendSQL is called.

If you disagree with my patch, please deny review.

Comment 8

[Bradley Baetz (:bbaetz)]

I'm not going to deny it - conceptually its the right thing to do. I'm just
warning that something may break. Although most stuff has been moved away from
that code, so its less of a risk than it was a couple of years ago.

Comment 9

[Frédéric Buclin]

requesting approval per bbaetz's previous comment.

Comment 10

[Dave Miller [:justdave]]

Comment on attachment 191334 [details] [diff] [review]
patch for all versions >= 2.17.4

ok, let's do it.  It's obviously the right thing to do, and this routine will
be gone before mod_perl sees the light of day anyway.  Just make sure we QA the
heck out of it. :)

Comment 11

[Frédéric Buclin]

tip:

Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.59; previous revision: 1.58
done

2.20rc1:

Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.56.2.2; previous revision: 1.56.2.1
done

2.18.3:

Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.12.2.3; previous revision: 1.12.2.2
done