484206 - Software error when granting a user access to a large number of groups

Status: NEW

(Bugzilla :: Administration, task)

Description

[Eric Olson]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.7) Gecko/2009021910 Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1)
Build Identifier: 3.2.2

When I grant a user access to a large number of groups, Bugzilla throws an error. I suspect this is due to the INSERT INTO profiles_activity action on line 315 of editusers.cgi. If the string of group names becomes long, it no longer fits into the 255 character limit of the newvalue column of profiles_activity.

Workaround: grant access to a smaller number of groups at a time. Repeat until all groups have been granted.

Reproducible: Always

Comment 1

[Eric Olson]

Attachment: Full text of the software error page

Comment 2

[Max Kanat-Alexander]

Oh yeah, we're probably just not splitting that data properly.

Comment 3

[Egil Kvaleberg]

Almost the same happens for me consistently if you try to change more than approx. 500 bugs at once. The error is:


  Software error:
  
  Not an ARRAY reference at Bugzilla/Object.pm line 201.

  For help, please send mail to the webmaster (root@localhost), giving this error message and the time and date of the error.

Comment 4

[Frédéric Buclin]

Bugzilla 3.2 is restricted to security bugs only. Moreover, this bug is either assigned to nobody or got no traction for several months now. Rather than retargetting it at each new release, I'm clearing the target milestone and the bug will be retargetted to some sensible release when someone starts fixing this bug for real (Bugzilla 3.8 more likely).

Comment 6

[Fida]

i've fixed that by changing the data type of columns oldvalue and newvalue (from table profile_activity) from charcater varying (255) to text. it is probably not a good idea, but this helped me to avoid this bug.

Comment 8

[Simon Green]

Attachment: bug484206-v1.patch

You might be wondering why I added the start/commit transaction statements. During my testing, it turned out that the two rows in the profiles_activity table were one second apart. By doing it in a transaction, we can be assured that profiles_when will be the same for all rows.

Comment 9

[Frédéric Buclin]

Comment on attachment 8588425 [details] [diff] [review]
bug484206-v1.patch

>+++ b/Bugzilla/User.pm

>+            my $removed_list = join(', ', map { $_->name } @$removed);
>+            my $added_list   = join(', ', map { $_->name } @$added);
>+
>+            while ($removed_list || $added_list) {
>+                my ($removestr, $addstr) = ($removed_list, $added_list);
>+                if (length($removestr) > MAX_LINE_LENGTH) {
>+                    my $commaposition = find_wrap_point($removed_list, MAX_LINE_LENGTH);
>+                    $removestr = substr($removed_list, 0, $commaposition);
>+                    $removed_list = substr($removed_list, $commaposition);
>+                } else {
>+                    $removed_list = ""; # no more entries
>+                }
>+                if (length($addstr) > MAX_LINE_LENGTH) {
>+                my $commaposition = find_wrap_point($added_list, MAX_LINE_LENGTH);
>+                        $addstr = substr($added_list, 0, $commaposition);
>+                    $added_list = substr($added_list, $commaposition);
>+                } else {
>+                    $added_list = ""; # no more entries
>+                }

Instead of duplicating this code from Bugzilla::Bug::LogActivityEntry(), please refactor it into some subroutine in Bugzilla::Util which would return an arrayref.


>+                trick_taint($addstr);
>+                trick_taint($removestr);

No reason to detaint strings here. They are already detainted.



>+++ b/editusers.cgi

>+         GROUP BY profiles.login_name, profiles_activity.profiles_when, fielddefs.name

Use $dbh->sql_group_by().

Comment 10

[Simon Green]

Attachment: bug484206-v2.patch

With the suggestions LpSolit mentioned.

Comment 11

[Frédéric Buclin]

Comment on attachment 8588489 [details] [diff] [review]
bug484206-v2.patch

>+++ b/Bugzilla/Util.pm

>+=item C<split_strings($length, @strings)>
>+
>+Splits an arbitary number of strings so each is a maximum of $max_length

$length or $max_length? :)

Also, please include a test in t/007util.t for this new method, to make sure it works as expected.

Comment 12

[:glob ✱]

Comment on attachment 8588489 [details] [diff] [review]
bug484206-v2.patch

Review of attachment 8588489 [details] [diff] [review]:
-----------------------------------------------------------------

r- as per issues raised by lpsolit in comment 11

::: Bugzilla/User.pm
@@ +136,5 @@
>  }
>  
> +# Used in _update_groups(). Gives the max length of lines in the
> +# profiles_activity table.
> +use constant MAX_LINE_LENGTH => 254;

this should be part of the 'constants' section, not 'functions'.
move it up about 25 lines.

Comment 13

[Simon Green]

Attachment: bug484206-v3.patch

Since glob isn't doing upstream reviews any more, I'm assigning the review to Dylan.