importxml.pl ignores the maxattachmentsize and maxlocalattachment parameters when importing attachments

RESOLVED FIXED in Bugzilla 4.4

Status

()

Bugzilla
Bug Import/Export & Moving
P3
normal
RESOLVED FIXED
11 years ago
3 years ago

People

(Reporter: Yann, Assigned: Frédéric Buclin)

Tracking

unspecified
Bugzilla 4.4
Bug Flags:
approval +
approval4.4 +

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

11 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20061010 Firefox/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20061010 Firefox/2.0

Bugzilla version: 2.22.1

How can I tell importxml.pl to import files on local storage instead of DB?

I use the encoding "filename" for the XML, and put all my files to import inside a folder that I give as parameter (--attach_data = ...).

Here is a sample of my xml file:
--------------------------------

<attachment
              isobsolete="0"
              ispatch="0"
              isprivate="0">
            <attachid>302</attachid>
            <date>2005-07-27 09:49:29 CET</date>
            <desc>Screen shots CR 1426</desc>
            <filename>Screen shot.doc</filename>
            <type>application/msword</type>
            <data encoding="filename">CR1426-20050727094924.doc</data>        
          </attachment> 

I tried to customize import.xml, but I fail to insert a null value into the 'thedata' field on Mysql (though I think this is needed so that it knows it is on local storage). I took out the code from attachment.cgi.

Below is my failing patch:
--------------------------

93a94
> use File::Copy;
418a420
> 		$attachment{'filepath'} = $attach_filename; #YC
1143c1145,1146
<         my $att_data = $att->{'data'};
---
>         my $att_data; #YC
> 	trick_taint($att_data);
1146c1149
<         trick_taint($att_data);
---
>         # YC trick_taint($att_data);
1148a1152,1159
> 	
> 	my $attachdir = "d:/bugzilla/bugzilla/data/attachments";
> 	my $hash_yc = ($att_id % 100) + 100;
> 	$hash_yc =~ s/.*(\d\d)$/group.$1/;
> 	mkdir "$attachdir/$hash_yc", 0770;
> 	chmod 0770, "$attachdir/$hash_yc";
> 	copy($att->{'filepath'}, ">$attachdir/$hash_yc/attachment.$att_id");

Reproducible: Always

Comment 1

11 years ago
The rewrite of importxml happened at about the same time as the local storage became an option, which meant I wasn't thinking of it when I did it ;)

What importxml should do is check the maxlocalattachmentsize paramater and respect it. The code in attachment.cgi and Attachment.pm can give you a better idea of how this works. If you want to write a patch, feel free to do so and attach it to this bug and request me as the reviewer. 
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 2

11 years ago
Created attachment 245754 [details] [diff] [review]
Patch v1 (against 2.22.1+)

This should be working, added a "--local_storage|-l" option on the command line.
Attachment #245754 - Flags: review?(ycombarnous)
Attachment #245754 - Flags: review?(ycombarnous) → review?(ghendricks)
(Assignee)

Comment 3

11 years ago
Comment on attachment 245754 [details] [diff] [review]
Patch v1 (against 2.22.1+)

please create a valid and readable patch with "diff -uN". There must be at least 3 lines of context.
Attachment #245754 - Flags: review?(ghendricks) → review-
(Reporter)

Comment 4

11 years ago
Created attachment 246157 [details] [diff] [review]
Patch v1.1 (against 2.22.1)

Patch created with cvs.
Attachment #245754 - Attachment is obsolete: true
(Reporter)

Updated

11 years ago
Attachment #246157 - Flags: review?(LpSolit)
(Assignee)

Comment 5

11 years ago
I would prefer a patch against 2.23.3+ as no enhancement bug will be taken on the 2.22 branch. Your patch will be delayed till 3.2 anyway as the code is frozen to prepare Bugzilla 3.0.
Severity: normal → enhancement

Comment 6

11 years ago
Comment on attachment 246157 [details] [diff] [review]
Patch v1.1 (against 2.22.1)

>@@ -79,7 +79,7 @@
> use Bugzilla::Component;
> use Bugzilla::Milestone;
> use Bugzilla::FlagType;
>-use Bugzilla::Config qw(:DEFAULT $datadir);
>+use Bugzilla::Config qw(:DEFAULT $attachdir);
> use Bugzilla::BugMail;
> use Bugzilla::User;
> use Bugzilla::Util;

Config now provides a bz_locations function. Use this instead.


>@@ -1140,12 +1148,30 @@
>             $att->{'desc'}, $att->{'ctype'}, $att->{'ispatch'},
>             $att->{'isprivate'}, $att->{'isobsolete'}, $exporterid);
>         my $att_id   = $dbh->bz_last_key( 'attachments', 'attach_id' );
>-        my $att_data = $att->{'data'};
>+        my $att_data; 
>+	if ($local_storage) {
>+ 	    $att_data = $att_data || '';
>+ 	}
>+ 	else {
>+	    $att_data = $att->{'data'};
>+ 	}

What is the point of this? $att->{'data'} is the data read from the file. It should make no difference whether it is being stored in the database or locally.

>         my $sth = $dbh->prepare("INSERT INTO attach_data (id, thedata) 
>                                  VALUES ($att_id, ?)" );
>         trick_taint($att_data);
>         $sth->bind_param( 1, $att_data, $dbh->BLOB_TYPE );
>         $sth->execute();

So we are storing in both the database and on the filesystem? shouldn't there be an if block otherwise?

As LpSolit said, this really needs to be taken against the tip. 2.22 branch is closed to new features.
Attachment #246157 - Flags: review?(LpSolit) → review-

Comment 7

11 years ago
(In reply to comment #6)
> (From update of attachment 246157 [details] [diff] [review] [edit])
> >@@ -79,7 +79,7 @@
> > use Bugzilla::Component;
> > use Bugzilla::Milestone;
> > use Bugzilla::FlagType;
> >-use Bugzilla::Config qw(:DEFAULT $datadir);
> >+use Bugzilla::Config qw(:DEFAULT $attachdir);
> > use Bugzilla::BugMail;
> > use Bugzilla::User;
> > use Bugzilla::Util;
> 
> Config now provides a bz_locations function. Use this instead.
> 
> 
> >@@ -1140,12 +1148,30 @@
> >+	if ($local_storage) {
> >+ 	    $att_data = $att_data || '';
> >+ 	}
> >+ 	else {
> >+	    $att_data = $att->{'data'};
> >+ 	}
> 
> What is the point of this? $att->{'data'} is the data read from the file. It
> should make no difference whether it is being stored in the database or
> locally.

Ah I think I get it now. Since the database needs a 0 byte attachment in order to know to check the local files.
> 
> >         my $sth = $dbh->prepare("INSERT INTO attach_data (id, thedata) 
> >                                  VALUES ($att_id, ?)" );
> >         trick_taint($att_data);
> >         $sth->bind_param( 1, $att_data, $dbh->BLOB_TYPE );
> >         $sth->execute();
> 
> So we are storing in both the database and on the filesystem? shouldn't there
> be an if block otherwise?
> 

That would explain this as well.

> As LpSolit said, this really needs to be taken against the tip. 2.22 branch is
> closed to new features. 
> 

This is still true however. :)
(Reporter)

Comment 8

11 years ago
(In reply to comment #7)
> Ah I think I get it now. Since the database needs a 0 byte attachment in order
> to know to check the local files.

Correct. All I did is taking code from attachment.cgi, miming exactly what it is doing. This is in the "validateData" function:

 if (!$cgi->param('bigfile'))
  {
      # enable 'slurp' mode
      local $/;
      $data = <$fh>;
  }

  $data
    || ($cgi->param('bigfile'))
    || ThrowUserError("zero_length_file");

  return $data || '';

> > >         my $sth = $dbh->prepare("INSERT INTO attach_data (id, thedata) 
> > >                                  VALUES ($att_id, ?)" );
> > >         trick_taint($att_data);
> > >         $sth->bind_param( 1, $att_data, $dbh->BLOB_TYPE );
> > >         $sth->execute();
> > 
> > So we are storing in both the database and on the filesystem? shouldn't there
> > be an if block otherwise?
> > 
> 
> That would explain this as well.

Yep.

>> As LpSolit said, this really needs to be taken against the tip. 2.22 branch is closed to new features. 
> This is still true however. :)

I will check for porting this to HEAD.

However, be reminded that the -attach_data option in importxml.pl (for reading local files instead of base64) appeared in 2.22.1 version, not in 2.22 ;-).
(Reporter)

Comment 9

11 years ago
(In reply to comment #8)
> However, be reminded that the -attach_data option in importxml.pl (for reading
> local files instead of base64) appeared in 2.22.1 version, not in 2.22 ;-).
> 
Actually, meant "attach_path" option, not attach_data.
(Reporter)

Comment 10

11 years ago
>+=item B<-m>
>+
>+    Store attachment on local filesystem (big file).

Actually, should be +=item B<-l>
(Reporter)

Comment 11

11 years ago
Created attachment 246899 [details] [diff] [review]
Patch v1.2 (against 2.23.3+)

Patch against 2.23.3+ this time.
Should be ok now
Attachment #246157 - Attachment is obsolete: true
Attachment #246899 - Flags: review?(ghendricks)

Comment 12

11 years ago
Comment on attachment 246899 [details] [diff] [review]
Patch v1.2 (against 2.23.3+)

This looks good. I am not sure what the status of the tip is as for being frozen.
Attachment #246899 - Flags: review?(ghendricks) → review+

Updated

11 years ago
Flags: approval?
(Assignee)

Comment 13

11 years ago
Comment on attachment 246899 [details] [diff] [review]
Patch v1.2 (against 2.23.3+)

>Index: importxml.pl

>+	    if (length($attachment{'data'})>$limit) {

Nit: add whitespaces around ">".


>+        my $att_data;
>+	if ($local_storage) {
>+ 	    $att_data = $att_data || '';

You already know that $att_data is undefined, so this should be: $att_data = '';

>+ 	}
>+ 	else {
>+	    $att_data = $att->{'data'};
>+ 	}

This IF block should be written: my $att_data = $local_storage ? '' : $att->{'data'};


>+	    my $attachdir = bz_locations()->{'attachdir'};
>+            my $hash = ($att_id % 100) + 100;
>+            $hash =~ s/.*(\d\d)$/group.$1/;
>+            mkdir "$attachdir/$hash", 0770;
>+            chmod 0770, "$attachdir/$hash";
>+            open(AH, ">$attachdir/$hash/attachment.$att_id");
>+	    binmode AH;

This part of the code is duplicated; it already exists as is in Attachment.pm. You should refactor this code in a routine such as create_local_attachment() or prepare_local_attachment() or some other explicit name. That's the main reason for my r-.
Attachment #246899 - Flags: review-
(Assignee)

Updated

11 years ago
Assignee: import-export → ycombarnous
Flags: approval?
Target Milestone: --- → Bugzilla 3.2

Comment 14

9 years ago
Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set the "blocking3.2" flag to "?". Then, either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.

This particular bug has not been touched in over eight months, and thus is being retargeted to "---" instead of "Bugzilla 4.0". If you believe this is a mistake, feel free to retarget it to Bugzilla 4.0.
Target Milestone: Bugzilla 3.2 → ---

Updated

8 years ago
Priority: -- → P3
Whiteboard: [needs new patch]
(Assignee)

Updated

7 years ago
Assignee: ycombarnous → import-export
(Assignee)

Comment 15

3 years ago
This is actually a bug. importxml.pl bypasses checks done by Bugzilla::Attachment->create as it does it own stuff. Everything is hardcoded in importxml.pl. It's time for it to call new methods such as Bugzilla::Bug->create and Bugzilla::Attachment->create.

(in a separate bug, it's also time to use Bugzilla::Error instead of self-made error messages.)
Severity: enhancement → normal
Summary: Enable importxml to import attachment on local storage (versus DB) → importxml.pl ignores the maxattachmentsize and maxlocalattachment parameters when importing attachments
Whiteboard: [needs new patch]
(Assignee)

Updated

3 years ago
Assignee: import-export → LpSolit
Status: NEW → ASSIGNED
Target Milestone: --- → Bugzilla 4.4
(Assignee)

Comment 16

3 years ago
Created attachment 8355720 [details] [diff] [review]
patch, v2

The code is working fine except for one thing:

If the attachment is larger than maxattachmentsize, then Bugzilla::Attachment->create will store the attachment locally, as expected. But the problem is that it calls "chmod 0770" and depending on the UID and GID used when running importxml.pl, the directory and the file can be unreadable by the web server (permission denied). Once we run checksetup.pl or contrib/fixperms.pl, the attachment is readable again, but I'm not sure that a good workaround.
Attachment #246899 - Attachment is obsolete: true
(Assignee)

Comment 17

3 years ago
Created attachment 8355721 [details] [diff] [review]
patch, v2.1

Cleaner patch. But the permission issue is still there.
Attachment #8355720 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8355720 - Attachment description: bug0-9.diff → patch, v2
(Assignee)

Updated

3 years ago
Attachment #8355721 - Attachment description: patch, v1.1 → patch, v2.1
(Assignee)

Comment 18

3 years ago
<justdave> probably should advise to run it as the webserver user then

So not something to do in 4.4.
Target Milestone: Bugzilla 4.4 → Bugzilla 5.0
I think we can take it for 4.4 on the following grounds:

1) The current situation means if you submit a large attachment it breaks
2) The situation with this fix is that if you submit a large attachment it breaks unless you happen to be running the script as the correct user.

#2 is arguably a better situation, since you can then correct your wrapper scripts or whatever to run as the correct user and have it fixed.
Target Milestone: Bugzilla 5.0 → Bugzilla 4.4
For clarification, the reason NOT to take it is that it potentially required configuration changes on your system, before before it didn't matter what user it ran as as long as it could read the config, and now *if* it needs to write an attachment, it also needs to be able to write to disk.  If the script worked at all before with large attachments, then this is a bad thing, because it would break people who weren't running it as the correct user.  But since it's already breaking anyway, and the fix lets you unbreak it if you use the correct user (where you couldn't unbreak it before), this is okay.
(Assignee)

Comment 21

3 years ago
Comment on attachment 8355721 [details] [diff] [review]
patch, v2.1

In that case the patch is ready. :)
Attachment #8355721 - Flags: review?(dkl)
(Assignee)

Comment 22

3 years ago
Created attachment 8355768 [details] [diff] [review]
patch, v2.2

Forgot to include the isobsolete attribute.
Attachment #8355721 - Attachment is obsolete: true
Attachment #8355721 - Flags: review?(dkl)
Attachment #8355768 - Flags: review?(dkl)
Comment on attachment 8355768 [details] [diff] [review]
patch, v2.2

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

r=dkl
Attachment #8355768 - Flags: review?(dkl) → review+

Updated

3 years ago
Flags: approval?
Flags: approval4.4?
Flags: approval?
Flags: approval4.4?
Flags: approval4.4+
Flags: approval+
(Assignee)

Comment 24

3 years ago
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified importxml.pl
Committed revision 8858.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.4/
modified importxml.pl
Committed revision 8648.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Comment 25

3 years ago
Added to relnotes for 4.4.2.
You need to log in before you can comment on or make changes to this bug.