Last Comment Bug 525025 - Custom field names having UTF8-characters in them crash Bugzilla
: Custom field names having UTF8-characters in them crash Bugzilla
Status: RESOLVED FIXED
: intl
Product: Bugzilla
Classification: Server Software
Component: Creating/Changing Bugs (show other bugs)
: 3.5
: All All
: -- major (vote)
: Bugzilla 3.4
Assigned To: Frédéric Buclin
: default-qa
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-28 11:52 PDT by Frédéric Buclin
Modified: 2010-02-28 10:48 PST (History)
6 users (show)
mkanat: approval+
mkanat: approval3.4+
mkanat: blocking3.4.3+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch, v1 (1.39 KB, patch)
2009-10-28 11:52 PDT, Frédéric Buclin
mkanat: review-
Details | Diff | Splinter Review
patch for 3.4.3, v2 (727 bytes, patch)
2009-10-28 18:02 PDT, Frédéric Buclin
mkanat: review+
Details | Diff | Splinter Review
patch for tip, v3 (2.04 KB, patch)
2009-11-01 11:29 PST, Frédéric Buclin
mkanat: review+
Details | Diff | Splinter Review

Description Frédéric Buclin 2009-10-28 11:52:55 PDT
Created attachment 408892 [details] [diff] [review]
patch, v1

Create a custom field name with UTF-8 characters in it, e.g. "cf_déjà_עברית". The creation succeeds, and you can add values to this field without any problem.

Now file a new bug and submit it:

invalid bug attribute cf_déjà _עברית at Bugzilla/Bug.pm line 3668
	Bugzilla::Bug::AUTOLOAD('Bugzilla::Bug=HASH(0x95e5cb0)') called at Bugzilla/Bug.pm line 811
	Bugzilla::Bug::update('Bugzilla::Bug=HASH(0x95e60e0)', '2009-10-28 19:47:05') called at /var/www/html/bugzilla/post_bug.cgi line 249


Now when you fix AUTOLAOD, you realize you cannot update this custom field in the bug you filed. No error is thrown, and all other changes are committed correctly, but this field remains unaltered. That's because CGI.pm seems to need UTF-8 parameter names to be encoded.
Comment 1 Max Kanat-Alexander 2009-10-28 17:24:59 PDT
Comment on attachment 408892 [details] [diff] [review]
patch, v1

>+        if (Bugzilla->params->{'utf8'} && $_[0] =~ /^cf_/) {
>+            utf8::encode($_[0]) if utf8::is_utf8($_[0]);
>+        }

  That will change it in the caller, which may not be what we intend.


  In actual fact, we should not be allowing UTF-8 column names. I'm surprised they work at all.
Comment 2 Frédéric Buclin 2009-10-28 18:02:00 PDT
Created attachment 408998 [details] [diff] [review]
patch for 3.4.3, v2

Do you prefer this? :)
Comment 3 Max Kanat-Alexander 2009-10-28 18:03:51 PDT
Comment on attachment 408998 [details] [diff] [review]
patch for 3.4.3, v2

Yeah, that looks great.

But what do we do for installations that already have UTF-8 column names? Just tell them to delete them?
Comment 4 Max Kanat-Alexander 2009-10-28 18:04:55 PDT
I suppose deleting them is reasonable, since they never could have been updated, right?
Comment 5 Frédéric Buclin 2009-10-28 18:07:37 PDT
(In reply to comment #4)
> I suppose deleting them is reasonable, since they never could have been
> updated, right?

Right. No bugs can have these fields set (as post_bug.cgi crashes and process_bug.cgi ignores these fields), and so they can easily mark these fields as obsolete and delete them.
Comment 6 Vitaly Fedrushkov 2009-10-29 04:20:48 PDT
IMHO this had its chance to work: 

1) before bug 363153;

2) with some old CGI.pm version (like ТТ 2.16..2.19, one of which most likely caused bug 457524)

I've also asked localizers@bugzilla.org for feedback.
Comment 7 Max Kanat-Alexander 2009-10-29 05:39:47 PDT
(In reply to comment #6)
> IMHO this had its chance to work: 
> 
> 1) before bug 363153;

  Hmm. Except, at that time, \w+ on the string would only have matched ASCII A-Za-z_ characters. So if you had a Unicode sequence that translated exactly to those (is that even possible?) it would have worked, but otherwise not.

> 2) with some old CGI.pm version (like ТТ 2.16..2.19, one of which most likely
> caused bug 457524)

  Mmm, we did the CGI.pm param() translation to utf8 ourselves as part of bug 363153.
Comment 8 Frédéric Buclin 2009-10-29 05:43:20 PDT
Comment on attachment 408998 [details] [diff] [review]
patch for 3.4.3, v2

As discussed on IRC, we will take this minimal change on the 3.4 branch, but on HEAD we are going to let checksetup.pl remove invalid custom field names.

So we will still need to relnote it for 3.4.3 as we won't delete them automatically.
Comment 9 Vitaly Fedrushkov 2009-10-29 05:49:26 PDT
It "could" work, with some UTF8-ignorant-but-safe environment including certain
browsers, CGI/TT module versions, Apache/squid settings combinations...

My point is not to be so certain in our intentions to delete such fields.  

Perhaps we're facing regression introduced by bug 363153.  Then attachment
408892 [review] is probably a valid approach as well.
Comment 10 Frédéric Buclin 2009-10-29 05:53:51 PDT
(In reply to comment #9)
> It "could" work, with some UTF8-ignorant-but-safe environment

But the custom field name is also used as DB column name, and I'm not sure all DBs support all UTF8 characters in their name. So it's still safer to reject those characters.
Comment 11 Frédéric Buclin 2009-11-01 11:29:09 PST
Created attachment 409608 [details] [diff] [review]
patch for tip, v3
Comment 12 Max Kanat-Alexander 2009-11-01 11:37:51 PST
Comment on attachment 409608 [details] [diff] [review]
patch for tip, v3

Looks good to me! :-)
Comment 13 Frédéric Buclin 2009-11-01 11:51:55 PST
tip:

Checking in Bugzilla/Field.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v  <--  Field.pm
new revision: 1.47; previous revision: 1.46
done
Checking in Bugzilla/Install/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/DB.pm,v  <--  DB.pm
new revision: 1.74; previous revision: 1.73
done


3.4.2:

Checking in Bugzilla/Field.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v  <--  Field.pm
new revision: 1.43.2.1; previous revision: 1.43
done
Comment 14 Max Kanat-Alexander 2009-11-05 04:16:25 PST
In the interest of doing the releases while I have time, I added this bug to the release notes and checked in the release note item without review.

tip:

Checking in template/en/default/pages/release-notes.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/pages/release-notes.html.tmpl,v  <--  release-notes.html.tmpl
new revision: 1.42; previous revision: 1.41
done

3.4.3:

Checking in template/en/default/pages/release-notes.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/pages/release-notes.html.tmpl,v  <--  release-notes.html.tmpl
new revision: 1.33.2.9; previous revision: 1.33.2.8
done
Comment 15 Zibi Braniecki [:gandalf][:zibi] 2009-11-08 08:51:34 PST
we suspect this change blocked us from creating custom searches with UTF8 char names.

Is that possible reason? (not sure if custom search is a custom field :)).

We updated bugs.aviary.pl to 3.4.3 and noticed it immediately.
Comment 16 Frédéric Buclin 2009-11-08 10:58:06 PST
(In reply to comment #15)
> we suspect this change blocked us from creating custom searches with UTF8 char
> names.

You cannot have UTF8 characters in custom field names (while UTF8 characters are still allowed in custom field descriptions, which is what the user see) as they crash Bugzilla. If you already had such characters in custom field names, you are in trouble already.
Comment 17 Vitaly Fedrushkov 2009-11-08 19:29:37 PST
(In reply to comment #15)
> we suspect this change blocked us from creating custom searches with UTF8 char
> names.

How exactly this fails? Have a look at bug 477513.
Comment 18 Vitaly Fedrushkov 2009-11-08 20:12:03 PST
(In reply to comment #15)
> we suspect this change blocked us from creating custom searches with UTF8 char
> names.

Unable to reproduce on landfill:

https://landfill.bugzilla.org/bugzilla-3.4-branch/buglist.cgi?cmdtype=runnamed&namedcmd=%E3%82%8F%E3%81%9F%E3%81%97%E3%81%AE%20bug%20|%20%D0%9C%D0%BE%D0%B8%20%D0%BE%D1%88%D0%B8%D0%B1%D0%BA%D0%B8
Comment 19 Frédéric Buclin 2009-11-09 02:11:13 PST
That's totally unrelated to this bug. Custom searches and custom field names have nothing to do together. This bug is about custom field names.

Note You need to log in before you can comment on or make changes to this bug.