194125 - CGI.pl warning: Character in "c" format wrapped...

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[Peter Pundy]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0)
Build Identifier: 

My bugzilla server just had its disk filled with apache error log containing 
this message:

[Wed Feb 19 17:09:16 2003] [error] [client 192.168.0.44] [Wed Feb 19 17:09:16 
2003] attachment.cgi: Character in "c" format wrapped at CGI.pl line 83., 
referer: http://bugzilla.optiwave.com/dev/attachment.cgi


Reproducible: Didn't try

Steps to Reproduce:
Sorry, haven't got a non-production platform to attempt reproduction yet.
Actual Results:  
No reproduction results yet, sorry.

Expected Results:  
CGI.pl / attachment.cgi shouldn't produce errors so frequently as to fill a 1G 
partition.

Line 83 code of CGI.pl is:

$todecode =~ s/%([0-9a-fA-F]{2})/pack("c",hex($1))/ge;

I could be wrong, but hex() returns an unsigned value 0-255 yes?  Which should 
be matched with a "C" format, not a "c" format.

That's the patch I'm testing now.  (I'm using v2.16.2)

Comment 1

[Bradley Baetz (:bbaetz)]

Hmm. This should only be an issue with non-ascii names, and we don't really
support that. Is that the case for you? What perl version are you using?

Current CGI.pm (which CVS-HEAD bugzilla uses) uses just uses |chr| instead of
pack. We should probably do that too.

Comment 2

[Peter Pundy]

Some additional facts:
- we use perl v5.8.0
- there were over 2 million lines in the log file
- only a couple hundred did not contain 'Character "c" format wrapped
- the last 1000 lines were all the error message and occurred over the space of 
2.5 minutes

Now, do you mean non-ascii filenames, user names?  I did change the system 
locale semi-recently from en_US.utf8 to en_US.iso885915

My wonder is what operation would pass over that CGI.pl line enough times to 
generate a couple million warnings in an hour or so?

Comment 3

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

Strange, I just had this happen on a test install when I tested a relatively
simple patch (bug 193947), although in that case it might have been my fault (I
may have introduced an infinite loop).  Still, one has to wonder if the error
trapping code is at fault.

Comment 4

[Bradley Baetz (:bbaetz)]

We don't have any eror trapping code, really.

I reproduced this errro using |http://mango.home/bugzilla-2.16/?foo=%ff|. I
wonder if an attachment somehow went through url_decode. It shouldn't have.
anywya, heres not much point in looking into this too deeply, given that HEAD
fdoesn't use that code.

Comment 5

[Peter Pundy]

Myk/Bradley: when you say reproduced, do you mean you achieved in getting a 
single error message, or a significant number of them?  I, too, wondered if 
attachment data was somehow getting fed through url_decode.  

While it may not be worth investigating too much farther, if anyone has 
familiarity with the code area, it would be worthwhile to:

- comment on a patch for live production 2.16.2 versions consisting of chaning 
CGI.pl:83 pack("c" to pack("C"

- confirm that the HEAD version doesn't suffer from the same weakness even 
though it is coded differently (do the apis used in HEAD avoid this data 
conversion error?, is attachment data be processed by url_decode()?, is that 
supposed to happen?)

Comment 6

[Bradley Baetz (:bbaetz)]

I only saw the one error per character. This does not happen with a HEAD tree.

Attachments shouldn't be being sent via a url, so url_decode shouldn't be being
called. If you can track down a case where atachment data is ending up that way,
then tahts a separate issue (which may or may not be a bug, depending)

Instead of using pack, just use |chr|.

Comment 7

[Dave Miller [:justdave]]

Anyone have a patch for this yet?  We're going to have a 2.16.3 shortly, if we
want this included in it....

Comment 8

[Bradley Baetz (:bbaetz)]

Attachment: fix

OK, trivial fix attached. %ff in url params warns w/o this patch, but not with
it.

Comment 9

[Bradley Baetz (:bbaetz)]

-> me

Comment 10

[Dave Miller [:justdave]]

approved for 2.16 branch

Comment 11

[Bradley Baetz (:bbaetz)]

Fixed-on-branch; not needed on trunk

Comment 12

[Dave Miller [:justdave]]

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