View Issue Details

IDProjectCategoryView StatusLast Update
0020340mantisbtattachmentspublic2016-08-15 09:02
Reporterlesley.liu Assigned Todregad  
PriorityhighSeveritymajorReproducibilityalways
Status closedResolutionfixed 
Product Version1.2.16 
Target Version1.2.20Fixed in Version1.2.20 
Summary0020340: Attachment is saved to disk and database at the same time
Description
  1. Set [ $g_file_upload_method = DISK; ] and configure [ $g_absolute_path_default_upload_folder ] in config_inc.php
  2. Report an issue with attachment.
  3. Check the attachment in mantis_bug_file_table, the content is not null. However, the disk file is also existed in the folder.

3a. If access /admin/move_attachments_page.php, the attachment count is not 0.
3b. If administrator do 'move to disk', the 'diskfile' will be mapping to a new place which file size is 2 bytes. Thus no one can download correct file content.

query result the attachment:

<pre>
+----+--------+-------+-------------+----------------------------------+--------------+---------------------------------------+----------+-----------+---------+------------+---------+
| id | bug_id | title | description | diskfile | filename | folder | filesize | file_type | content | date_added | user_id |
+----+--------+-------+-------------+----------------------------------+--------------+---------------------------------------+----------+-----------+---------+------------+---------+
| 5 | 4 | | | b66c36fbdc246d15ca50cb51073b0aca | tempfile.PNG | /volume1/web/wnbu/mantis-attachments/ | 17893 | image/png | '' | 1449109357 | 2 |
+----+--------+-------+-------------+----------------------------------+--------------+---------------------------------------+----------+-----------+---------+------------+---------+
</pre>

TagsNo tags attached.

Relationships

related to 0020351 new Enable schema updates in more than one branch 
related to 0020540 assigneddregad Implement upgrade step to cleanup corrupt disk attachments after db->disk conversion 

Activities

atrol

atrol

2015-12-06 05:59

developer   ~0052023

Not able to reproduce in current master (1.3.0-rc.1)

dregad

dregad

2015-12-06 06:07

developer   ~0052024

You can't reproduce the issue but you confirm it, does this means it applies to 1.2.x ?

atrol

atrol

2015-12-06 06:24

developer   ~0052027

The issues is reproducible in 1.2.19 but does no longer appear in current master.
The fix is in https://github.com/mantisbt/mantisbt/commit/02bbb995 which removes the two quotation marks from $c_content.

dregad

dregad

2015-12-06 08:58

developer   ~0052028

Ok, thanks for the clarification. I'll backport this commit then.

dregad

dregad

2015-12-06 11:05

developer   ~0052029

Last edited: 2015-12-06 11:36

I believe this is a regression that was introduced in 1.2.16 (commit 4f4e69bd7) when db_query() call was replaced by db_query_bound().

dregad

dregad

2015-12-06 12:29

developer   ~0052030

The fix is easy enough, but I'm wondering about the consequences of the regression, i.e. all the instances out there relying on DISK for attachments storage and potentially have a large number of invalid blobs in their mantis_bug_file_table.

This can be addressed by a simple SQL statement updating all blobs containing "''" and changing that to ''.

The "clean" way to handle this would be to write an install function, but I can't do this in 1.2.x as it would break the schema upgrade sequence. Any ideas ?

vboctor

vboctor

2015-12-06 13:22

manager   ~0052032

I would add a cleanup upgrade function in 1.3.0 so that this is cleaned up as part of upgrading to 1.3.0. I'm also OK adding schema upgrade for 1.2.20, we can revisit our process there given how long it takes between 1.2.0 and 1.3.0 for example.

vboctor

vboctor

2015-12-06 13:33

manager   ~0052033

Thinking about this more, we won't be able to add an upgrade function to 1.2.x since we identify upgrade steps by their sequence number now. So we can bundle the check / fix in the system utilities / check.php. We can potentially provide a warning to the administrator if they have blobs that needs cleanup when they are in the management area.

I would like to see us move (in 1.3.x) to a model where we track upgrade steps by an id rather than a sequence number. That would enable us to update schema in two branches in parallel. For examples, "api-tokens-1", "api-tokens-2", rather than 201 and 202.

atrol

atrol

2015-12-06 14:04

developer   ~0052034

In this special case we could add a step 184 in master-1.2.x as step 184 in master is
$g_upgrade[184] = array( 'UpdateFunction', 'do_nothing' );

vboctor

vboctor

2015-12-06 20:56

manager   ~0052035

Changing old upgrade steps typically ends up causing issues, so I would rather avoid it.

How about if we apply a code fix for 1.2.x that would treat this corrupted content values as empty blobs? And then clean such blobs when users upgrade to 1.3.x and hence, we don't need to code fix that goes only on 1.2.x.

lesley.liu

lesley.liu

2015-12-06 22:45

reporter   ~0052036

I have one 1.3.0-beta3 DB which doesn't have this issue. The change in core/file_api.php should fix it.

Is there a target date to provide the next stable release?

atrol

atrol

2015-12-07 01:48

developer   ~0052037

Last edited: 2015-12-07 02:48

Changing old upgrade steps typically ends up causing issues, so I would rather avoid it.

Of course, changes like this one 0006626:0029192 (using an index in master-1.2.x that is used for another purpose in master) will introduce trouble for some installations.
But I don't see this problem in the current case.
Not sure you got my idea.

We could add in master-1.2.x
$g_upgrade[184] = array( 'UpdateFunction', 'repair_attachments' );
and in master
$g_upgrade[202] = array( 'UpdateFunction', 'repair_attachments' );

This shouldn't introduce any issue as master will still be
$g_upgrade[184] = array( 'UpdateFunction', 'do_nothing' ); // DO NOTHING

atrol

atrol

2015-12-07 01:52

developer   ~0052038

Is there a target date to provide the next stable release?

No, but there is a newer 1.3.0-rc.1
https://www.mantisbt.org/bugs/changelog_page.php?version_id=245
http://sourceforge.net/projects/mantisbt/files/mantis-development/

dregad

dregad

2015-12-07 09:29

developer   ~0052040

Thanks for your feedback.

@vboctor

we won't be able to add an upgrade function to 1.2.x since we identify
upgrade steps by their sequence number

Exactly, this is why I asked for ideas.

bundle the check / fix in the system utilities / check.php

That would probably work actually, since it's not a blocking issue - users would only hit the problem when switching to DATABASE upload method or trying to move attachments to disk as the OP found out.

move (in 1.3.x) to a model where we track upgrade steps by an id rather than a sequence number

Well, I would argue that a sequence number is an ID...

Joke aside, I'm not seeing that much benefit of switching from a numeric, sequential ID to a string. I find it useful to have an ID which both identifies the upgrade step as well as where it belongs in the sequence. Furthermore, it would become complicated to identify which steps have been applied, when, and in which sequence.

That being said, maybe the ID doesn't need to be strictly sequential. In other words, as an alternative to your suggestion, we could "reserve" a block of upgrade steps when we switch to a new major version, allowing more flexibility for required schema changes within a minor release.

For example, an hypothetical 2.0.0 schema.php could look like this:
<pre>
$g_upgrade[201] = array( 'CreateIndexSQL', [...]

Release marker: 1.3.0

upgrade steps 202..219 are reserved for 1.3.x releases

$g_upgrade[220] = [...]

Release marker: 2.0.0

</pre>

The problem with this approach is that it would need some special processing to cover the following scenario :

  • user is running 2.0.0
  • we change 1.3.0 schema (e.g. adding step 202), release 1.3 and port the change to 2.x
  • we release 2.1.0
  • user upgrades to 2.1.0
    Current version of the installer would not do anything.

@atrol

In this special case we could add a step 184 in master-1.2.x as step 184 in master is
$g_upgrade[184] = array( 'UpdateFunction', 'do_nothing' );

Adding this only at step 184 would hit the same scenario I described above.

Considering your update in 0020340:0052037, adding the step again at 2020 would probably work in this specific case, because executing the upgrade function twice would not have any side effects.

As a side note,

$g_upgrade[184] = array( 'UpdateFunction', 'do_nothing' ); // DO NOTHING

is really useless - the upgrade step could simply be set to NULL (since 1.3.0-beta.1 / 061e66b5).

@lesley.liu

I have one 1.3.0-beta3 DB which doesn't have this issue

If you installed it straight from >= 1.30.0-beta.1 then you should be fine
If it was upgraded from >= 1.2.16 or an early 1.3.0dev release (before 02bbb995, i.e. around Oct 2013), then it is potentially affected.

lesley.liu

lesley.liu

2015-12-07 11:26

reporter   ~0052052

@atrol
Thank you. Since there are over 20 DBs in our server, my boss expects for the stable release. I put the updates in all file_api.php to stop non-null content increasing.

@dregad
We usually exact the release package and paste the existed config_inc.php when upgrade, guess the 1.3.0-beta3 DB should not be affected.
However, there is one 1.2.17 DB with some customized files does not have this issue. Not sure if the other changes done by my colleague avoid it.
That line in file_api.php is still use ---> $c_content = "''";

dregad

dregad

2015-12-07 11:44

developer   ~0052053

@lesley.liu

Just to clarify, the important part in my earlier comment is not how you installed the code but rather which version of Mantis was used to install or upgrade DB prior to the 1.3.0-beta/rc upgrade.

Anyway if you don't have any file attachments in the DB with just '' in the content field, then you're OK (this can be checked at DB level).

lesley.liu

lesley.liu

2015-12-07 12:17

reporter   ~0052058

@dregad

Yes, there is no '' existed in content of mantis_bug_file_table of that 2 DBs. It was only happened on 1.2.19 in our server.
Thanks for clarify.

atrol

atrol

2016-01-18 06:35

developer   ~0052325

Reminder sent to: dregad, vboctor

I think this issue is a blocking one for 1.3.

If we use upgrade step 184 (the one without any functionality at the moment) to cleanup the database we would have a fix for it in 1.2.20.
If we do the same again in a new step (would be 202 at the moment) we would also have the fix in 1.3 for those that installed 1.3 beta or rc versions.

vboctor

vboctor

2016-01-25 12:27

manager   ~0052378

My suggestion for end state is:

  1. Having fix that stops the bad setting of content in master and master-1.2.x
  2. Have code that detects bad content and deals with it in master-1.2.x
  3. Add an upgrade step to cleanup the records with bad content in master. No need for workaround code in 2.

I also suggest that we either open an issue with target version 1.3.x that matches the blocking filter so that we don't lose track of this for 1.3.0 release.

vboctor

vboctor

2016-07-10 03:21

manager   ~0053553

MantisBT 1.3.0 stable has been released. Not sure if there is still a need to worry about this for 1.2.x since users should upgrade to 1.3.0 stable.

atrol

atrol

2016-07-10 12:46

developer   ~0053557

The issue is fixed in 1.3.0 in a fresh install.
It's not fixed in 1.3.0 when upgrading from older versions where DISK is used to store attachments.
There are still invalid blobs in mantis_bug_file_table after upgrade.

dregad

dregad

2016-08-15 04:37

developer   ~0053805

With 1.3.0 now live, I don't think it's necessary anymore to add an admin check in 1.2.x to detect the situation.

Instead, we should focus on add a schema upgrade step (i.e. install function to repair corrupt attachments).

Considering that this issue tracks the fix of the root cause, i.e. the generation of the invalid attachments when converting from DB to DISK, I will resolve it again, and reopen 0020540 for follow-up in 1.3.x.

Related Changesets

MantisBT: master-1.2.x e9260b14

2015-12-06 06:59

dregad


Details Diff
Fix 2-byte attachment saved to DB when upload_method is DISK

Commit 4f4e69bd73e2840ce4542072bb544f51ddaf3cf7 changed file_add() to
use bind parameters, but failed to remove quotes escaping of $c_content
variable.

This results in an invalid 2-byte attachment (containing 2 single
quotes "''") being stored in the file table in addition to the one saved
to disk as expected, which does not actually have any impact unless the
admin tries to move attachments from DB to disk; in that case, the valid
files are overwritten, causing loss of data.

The problem is fixed by setting $c_content to an empty string.

Previously fixed in master branch commit
02bbb99501c48f89ad249fb505dc9bba93bae5c0

Fixes 0020340
Affected Issues
0020340
mod - core/file_api.php Diff File