223878 - Flag system dies when changing a deleted flag

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[Christian Reis]

Bizarre collision inside attachment.cgi. The following is a sequence of actions
that seemed to cause it:

1. I loaded, from bug 190042,
http://bugzilla.mozilla.org/attachment.cgi?id=134272&action=edit which had a
review request for me.
2. Vladd cancelled the review request.
3. I then set r- the patch and added a snide comment. I submitted the form, and
got back a 500 internal server error.
4. Somewhere between the above bryner also placed a review request, on bug
128673, for http://bugzilla.mozilla.org/attachment.cgi?id=134273&action=edit

Results:

- My r- was placed on attachment 134273 [details] [diff] [review]
- My comment was placed on the correct bug
- No r- was placed on attachment 134272 [details] [diff] [review]

Note that the attachment IDs are sequential.

Comment 1

[Christian Reis]

bmo log excerpt from my 500 ISE:

[Mon Oct 27 13:32:39 2003] attachment.cgi: Use of uninitialized value in
concatenation (.) or string at Bugzilla/FlagType.pm line 72.

[Mon Oct 27 13:32:39 2003] attachment.cgi: DBD::mysql::st execute
failed: You have an error in your SQL syntax.  Check the manual that
corresponds to your MySQL server version for the right syntax to use near '' at
line 1 at globals.pl line 279.

[Mon Oct 27 13:32:39 2003] [error] [client 200.206.134.238] malformed
header from script. Bad header=<p>: /opt/webtools/bugzilla/attachment.cgi

[Mon Oct 27 13:32:39 2003] attachment.cgi: SELECT 1, flagtypes.id,
flagtypes.name, flagtypes.description, flagtypes.cc_list,
flagtypes.target_type, flagtypes.sortkey, flag
types.is_active, flagtypes.is_requestable, flagtypes.is_requesteeble,
flagtypes.is_multiplicable FROM flagtypes WHERE flagtypes.id = : You
have an error in your SQL syntax
.  Check the manual that corresponds to your MySQL server version for
the right syntax to use near '' at line 1 at globals.pl line 284.

Comment 2

[[e-mail]]

I got a 500 trying to update attachment 134316 [details] [diff] [review] to bug 223943, even after
clicking back once and resubmitting; I then reloaded bug 223943 to find that my
changes had in fact taken effect, although I can't tell if any other attachments
were affected.

Comment 3

[Christian Reis]

Neil's IP address was 80.175.250.218 at the time. Myk, can you check the bmo log
and see if it's the same error I got?

Comment 4

[Myk Melez [:myk] [@mykmelez]]

[Tue Oct 28 06:59:00 2003] [error] [client 80.175.250.218] malformed header from
script. Bad header=No recipient addresses found i:
/opt/webtools/bugzilla/attachment.cgi
Use of uninitialized value in pattern match (m//) at CGI.pl line 450 (#1)

...

[Tue Oct 28 07:01:56 2003] attachment.cgi: Use of uninitialized value in
concatenation (.) or string at Bugzilla/FlagType.pm line 72.
[Tue Oct 28 07:01:56 2003] attachment.cgi: DBD::mysql::st execute failed: You
have an error in your SQL syntax.  Check the manual that corresponds to your
MySQL server version for the right syntax to use near '' at line 1 at globals.pl
line 279.
[Tue Oct 28 07:01:56 2003] [error] [client 80.175.250.218] malformed header from
script. Bad header=<p>: /opt/webtools/bugzilla/attachment.cgi
[Tue Oct 28 07:01:56 2003] attachment.cgi: SELECT 1, flagtypes.id,
flagtypes.name, flagtypes.description, flagtypes.cc_list, flagtypes.target_type,
flagtypes.sortkey, flagtypes.is_active, flagtypes.is_requestable,
flagtypes.is_requesteeble, flagtypes.is_multiplicable FROM flagtypes WHERE
flagtypes.id = : You have an error in your SQL syntax.  Check the manual that
corresponds to your MySQL server version for the right syntax to use near '' at
line 1 at globals.pl line 284.
Use of uninitialized value in pattern match (m//) at CGI.pl line 450 (#1)

Comment 5

[Myk Melez [:myk] [@mykmelez]]

I wonder if bug 183788, whose fix was recently applied to b.m.o, could have
anything to do with this.

Comment 6

[Dave Miller [:justdave]]

bmo log excerpt from a collision Jouni just made:

[Wed May 19 07:39:51 2004] attachment.cgi: Use of uninitialized value in
concatenation (.) or string at Bugzilla/FlagType.pm line 74.
[Wed May 19 07:39:51 2004] [error] [client 194.100.2.65] malformed header from
script. Bad header=</pre>: /opt/webtools/bugzilla/attachment.cgi
[Wed May 19 07:39:51 2004] attachment.cgi: DBD::mysql::st execute failed: You
have an error in your SQL syntax.  Check the manual that corresponds to your
MySQL server version for the right syntax to use near '' at line 1 [for
statement ``SELECT 1, flagtypes.id, flagtypes.name, flagtypes.description,
flagtypes.cc_list, flagtypes.target_type, flagtypes.sortkey,
flagtypes.is_active, flagtypes.is_requestable, flagtypes.is_requesteeble,
flagtypes.is_multiplicable FROM flagtypes WHERE flagtypes.id = '']) at
Bugzilla/DB.pm line 66
[Wed May 19 07:39:51 2004] attachment.cgi:      Bugzilla::DB::SendSQL('SELECT 1,
flagtypes.id, flagtypes.name, flagtypes.description, f...') called at
Bugzilla/FlagType.pm line 74
[Wed May 19 07:39:51 2004] attachment.cgi:      Bugzilla::FlagType::get(undef)
called at Bugzilla/Flag.pm line 639
[Wed May 19 07:39:51 2004] attachment.cgi:      Bugzilla::Flag::perlify_record()
called at Bugzilla/Flag.pm line 82
[Wed May 19 07:39:51 2004] attachment.cgi:      Bugzilla::Flag::get(62734)
called at Bugzilla/Flag.pm line 155
[Wed May 19 07:39:51 2004] attachment.cgi:     
Bugzilla::Flag::validate('HASH(0x82040f0)', 208847) called at
/opt/webtools/bugzilla/attachment.cgi line 151

Comment 7

[Jouni Heikniemi]

*** Bug 238711 has been marked as a duplicate of this bug. ***

Comment 8

[Jouni Heikniemi]

This sucks. It's probably a some sort of "flag midair" conflict, and I think we
should fix it for 2.18. 

On code level, the general idea seems to be that Flag->get returns an undef
value for the flag type, and perlify_record then promptly does a
FlagType->get(undef), causing a MySQL error. I have no idea how can Flag->get
return an undef - or what else could possibly be the cause... Anyone?

Comment 9

[Jouni Heikniemi]

Ok, I get it (finally ;-)). This occurs when you clear a flag, and
attachment.cgi (or show_bug) page tries to edit a flag with a certain ID. This
fails when somebody has already cancelled the flag (deleted it from the
database). Flag->validate also has a funny bug:

        # Make sure the flag exists.
        my $flag = get($id);
        $flag || ThrowCodeError("flag_nonexistent", { id => $id });

Code Error never gets thrown, as get dies when undef is returned as the
flag_type (in perlify_record called from get). Oh well. ;-)

Since Flag->create uses the SELECT MAX(ID) approach, this explains the scenario
in comment 0; a flag was cleared, and then a flag just created on another bug
(attachment) gets the same id. A form has already been loaded with the flag id
prior to cancellation, so any changes get saved on the wrong bug. This also
explains what happened in bug 208847 comment 19 (the request got Flag->migrated).


I think the correct fix would be to never delete flags (but rather mark them as
inactive). This way we would be able to find out the flag type afterwards and
create a new flag of the same type (or change the old one - I'm not sure what's
best here) even if the flag referenced in the form was already deleted. That
would also avoid the problem of ids getting reused.

Other solutions involve pumping extra data into the forms and using
identity-style (non-reusable) database ids.

Thoughts?

Comment 10

[Joel Peshkin]

I agree.  Never delete.

Comment 11

[Dave Miller [:justdave]]

Yes, we shouldn't provide ways to corrupt the database in a release version.

Comment 12

[Dave Miller [:justdave]]

would making the flag ID be an auto_increment work?  I don't see anything else
that could be construed as a primary key in that table...

Comment 13

[Jouni Heikniemi]

There are actually two issues here:

1) The flag system works incorrectly when the last used flag is deleted and then
another flag gets added. With some bad luck, flag edits on the former
bug/attachment get set on the new bug. This is comment 0.

2) The flag system dies with a 500 error when a flag is deleted (but the used id
isn't reused for another flag). This one is pretty well explained in comment 9.


I see two major approaches to the issue. In both cases, the key point is that we
need to make flag id's more unique than they are now - kill the reuse, that is. 

Fix model A is to make flag id an autoincrement field. This is good, with one
exception: case 2 is still problematic, unless we make attachment flag edit
forms also contain flagtypes, so that in case of a missing flag we'll be able to
create a new flag of the same type. This change will break localizations.

Fix model B is to stop deleting flags from the db when they get cleared. This
has some benefits, including the increasing amount of audit data. The ids become
automatically unique, and case 2 can be fixed by simply retrieving the flag
being set from the db; if it's deleted, just create a new flag of the same type.
If it's not, set the state of the existing one.

I'm drawn to model B (partially because I don't like deletes, partially because
of the localization stuff), but it involves another issue: we'd have to hack a
bit around whole Bugzilla to start ignoring deleted flags in searches, listings
and so on). I haven't gone through the changes needed. They won't be massive,
but it will introduce a certain bug risk.


Suggestions?

Comment 14

[Jouni Heikniemi]

Attachment: v1

Ok, here's the first stab. What this patch does is...

a) Add an is_active column to flags table; make Flag::exists depend on it 
   instead of an hardcoded constant value.

b) Make really many places contain a new constraint "is_active = 1" in one 
   form or another.

c) Make Flag::modify always set modified flags active, effectively 
   reviving possibly deleted ones. Also, make Flag::clear set is_active = 0
   instead of deleting.

d) Make Flag::perlify_record (and effectively, Flag::get as well) return undef
   if the flag id is invalid. This is the way it was supposed to work, but it
   never did (see comment 9 for details). It allows better error message in 
   case of a code bug; not really critical but was pretty easy to fix.

I bashed it pretty thoroughly, but of course, an additional testing run
wouldn't hurt.

Comment 15

[Joel Peshkin]

Comment on attachment 152253 [details] [diff] [review]
v1

fix the checksetup portion to do the AddField unconditionally and r=joel

Comment 16

[Dave Miller [:justdave]]

Comment on attachment 152253 [details] [diff] [review]
v1

r=justdave

Don't forget comment 15.  AddField already does the conditional inside the sub,
you don't need to repeat it outside of it :)

Comment 17

[Dave Miller [:justdave]]

a=justdave

Comment 18

[Jouni Heikniemi]

Checksetup issue fixed prior to checkin.

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.58; previous revision: 1.57
done
Checking in editflagtypes.cgi;
/cvsroot/mozilla/webtools/bugzilla/editflagtypes.cgi,v  <--  editflagtypes.cgi
new revision: 1.7; previous revision: 1.6
done
Checking in request.cgi;
/cvsroot/mozilla/webtools/bugzilla/request.cgi,v  <--  request.cgi
new revision: 1.14; previous revision: 1.13
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.286; previous revision: 1.285
done
Checking in Bugzilla/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Attachment.pm,v  <--  Attachment.pm
new revision: 1.17; previous revision: 1.16
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.37; previous revision: 1.36
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.18; previous revision: 1.17
done
Checking in Bugzilla/FlagType.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/FlagType.pm,v  <--  FlagType.pm
new revision: 1.7; previous revision: 1.6
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.57; previous revision: 1.56
done