7233 - Editversions.cgi has a potential race resulting in duplicate versions

Status: RESOLVED FIXED

(Bugzilla :: Administration, task, P3)

Description

[steveha]

By accident, I entered a product/version combination twice in the versions
tables.  The database ought to be set up in a way that guarantees that this
cannot happen.  (I'm not sure how to do this, since I am not a SQL guru.  I
believe that you could declare that the two columns together make a primary key,
and that would do it.)

Comment 1

[Terry Weissman]

[e-mail] is the new owner of Bugzilla and Bonsai.  (For details,
see my posting in netscape.public.mozilla.webtools,
news://news.mozilla.org/38F5D90D.F40E8C1A%40geocast.com .)

Comment 2

[Tara Hernandez]

Chris, put your SQL hat on...

Comment 3

[Chris Yeh]

actually you do this by creating an index that forces unique values on columns.
i agree that this is a good thing to have, i can't think of any situation where 
it would be good to have multiple instances of product/version in the versions 
table. 

moving to assigned.

Comment 4

[Chris Yeh]

made schema slightly more retentive by requiring unique on product name.
editproducts.cgi and editversions.cgi both have checking to prevent duplicates.

Comment 5

[Dave Miller [:justdave]]

Just double-checking here, because that doesn't sound right...   what if you have 
more than one version of the same product?  :)   I have to have two or more rows 
in the table with the same product name in order to do that (they'd each have 
different version numbers).

Comment 6

[Chris Yeh]

oh dear. you're right. i obviously was _not_ thinking. I did a partial backout 
of the change...unique indexing is out, the not null change is still there.

credited you in the comments for catching my idiocy.

Comment 7

[leger]

Marking Verified.

Comment 8

[Dave Miller [:justdave]]

Why is this verified fixed?  This is what's in checksetup.pl for the version 
table right now:

$table{versions} =
   'value tinytext,
    program varchar(64) not null';

I don't see any uniqueness in there.

You can make both columns unique as a pair by grouping them in parenthesis in the 
definition.

$table{versions} =
   'value tinytext,
    program varchar(64) not null

    unique (value, program)';

What was backed out before is that the one column was made unique by itself, 
instead of doing it as a pair, so it wouldn't let you put more than one version 
on a product.

This needs to be reopened.

Comment 9

[Chris Yeh]

ugh. you can't index a blob type (tinytext). we'd need to change this to 
varchar(255) or something. 

Comment 10

[Matthew Tuck [:CodeMachine]]

I assume the fix here is to set up a compound unique index.

Comment 11

[Dave Miller [:justdave]]

moving to real milestones...

Comment 12

[Matthew Tuck [:CodeMachine]]

Whatever is done here should be duplicated for the milestones table I assume.

Comment 13

[Dave Miller [:justdave]]

Taking all of cyeh's Bugzilla bugs.

Comment 14

[Dave Miller [:justdave]]

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

Comment 15

[Christian Reis]

New product: bugzilla.

Comment 16

[Dave Miller [:justdave]]

this looks like it requires a schema change (changing a tinytext to a
varchar(255) was suggested here).  CCing a few of the people working on the xdb
sql syntax for comment.

Comment 17

[Dave Miller [:justdave]]

We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.

Comment 18

[Bradley Baetz (:bbaetz)]

And we should use real ids, too.

Comment 19

[Dave Miller [:justdave]]

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

Comment 20

[Joel Peshkin]

In 2.17.6, this seems to work.....    If I add the same version twice, I get...
"Adding new version
	  	
The version 'a' already exists. Please press Back and try again."

Comment 21

[Jeff Lawson]

Joel Peshkin, did you check whether there is actualy a unique index yet on 
the "versions" table ("values" column)?  The editversions.cgi explicitly tries 
to do a select before inserting, but that is not atomic and does not guarantee 
uniqueness in race conditions (such as when two users are trying to do it, or 
if the user double-clicks the submit button).

Comment 22

[Joel Peshkin]

The potential for a race caused by 2 users simultaneously adding versions is
there.  The double-click scenario is not very plausible.  The other concern,
mentioned in bug 191745, suggests that the database protect itself from users
manually fiddling with it.

It looks like the right thing to do is to either 
a) LOCK/UNLOCK around the test and the insert
or
b) Add a unique index and change the INSERT to INSERT IGNORE
[[ At the same time, we should change the versions.value to a varchar(64) to
match bugs.version ]]

bbaetz -- versioncache is supposed to go away.  Does that plan impact this
decision??

Comment 23

[Jeff Lawson]

option (b) would seem to be preferable, since it is quite reasonable to 
enforce this type of constraint at the schema level.  Note that changing 
versions.value from "tinytext" to "varchar(64)" is actually a necessary 
pre-requisite before creating a unique index on that column (as per comment #9)

Comment 24

[Bradley Baetz (:bbaetz)]

See bug 153540.

versioncache has nothing to do with versions, really

Comment 25

[Matthew Tuck [:CodeMachine]]

I'll take this one, when I port to my new administration architecture all the
edit*.cgi should be consistent including on this issue.

Comment 26

[Joel Peshkin]

Not likely to make 2.18.  Change it back if I'm wrong.

Comment 27

[Peter D. Stout]

When this bug is fixed, I would suggest you consider making the size of the
versions.value and milestones.value fields consistent.

I would also note that according to the MySQL documenation (Section 11.4.3) TEXT
columns stored in MyISAM tables can be indexed as of MySQL version 3.23.2, which
is significantly older than what Bugzilla 2.18 minimally requires.

Comment 28

[Shankar Unni]

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

Comment 29

[Frédéric Buclin]

Is it now fixed by the checkin of bug 190226?

Comment 30

[GavinS]

(In reply to comment #29)
> Is it now fixed by the checkin of bug 190226?

Nope, this not fixed yet.

Comment 31

[Max Kanat-Alexander]

This bug has not been touched by its owner in over six months, even though it is
targeted to 2.20, for which the freeze is 10 days away. Unsetting the target
milestone, on the assumption that nobody is actually working on it or has any
plans to soon.

If you are the owner, and you plan to work on the bug, please give it a real
target milestone. If you are the owner, and you do *not* plan to work on it,
please reassign it to [e-mail] or a .bugs component owner. If you are
*anybody*, and you get this comment, and *you* plan to work on the bug, please
reassign it to yourself if you have the ability.

Comment 32

[Frédéric Buclin]

Attachment: patch, v1

First convert the 'value' field from tinytext to varchar(64), consistently with
templates which allow 64 characters and bugs.version which is also defined as
varchar(64).
Then add an index to the 'versions' table.

Comment 33

[Max Kanat-Alexander]

Comment on attachment 182007 [details] [diff] [review]
patch, v1

Technically, in order for that UNIQUE constraint to fix this problem, value
would have to be NOT NULL.

If there are currently *any* null values in the "value" field, you are in for a
very difficult problem.

Comment 34

[Frédéric Buclin]

Attachment: patch, v2

I don't see how a value could be NULL. The actual UI does not let you do that.
So I simply added NOTNULL => 1.

Comment 35

[Max Kanat-Alexander]

Comment on attachment 182045 [details] [diff] [review]
patch, v2

OK, yeah, now this looks obviously correct.

Comment 36

[Frédéric Buclin]

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.401; previous revision: 1.400
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.29; previous revision: 1.28
done