[Audacity-devel] Reopen bug 451

Discussion:

Paul Licameli

2016-12-19 18:34:58 UTC

I discovered only today that fixes I made for memory leaks may have
invalidated the fix for the old, P5 bug, 451
http://bugzilla.audacityteam.org/show_bug.cgi?id=451#c16.

The problem was that when a project fails to open with parse errors, then
good block files were erased from disk causing data loss.

The original fix, I believe, spared the files but at the expense of memory
leaks.

I wrote an easy alternative fix that still spares the files and does not
leak memory, by doing something similar to what happens when the project
closes normally.

I have not yet tried to reproduce the problem and verify my fix. It
depends on invalid .aup fils after all.

I will make a branch called bug451 in my fork.

PRL

Paul Licameli

2017-02-23 18:51:55 UTC

Permalink

I spend some time today cleaning up my origin fork. I see this fix I wrote
two months ago, for reopened bug 451, was never merged. So I am reminding
team about it again. It is a possible data loss bug that I re-introduced,
incidentally to the memory managment fixes. But Gale has said he thinks
the likelihood of this occurrence is much less than it was when the bug was
filed.

I should have checked up on this before RC2 was made, sorry. Do we want to
put this in?

PRL

Thanks, Paul. The original bug was P2 and the P5 was just for the residual
ASSERT confined to debug builds.
If those bad projects attached to the bug report cause data loss again in
release builds, I'm guessing it's P3 (a grade lower than before because
there
are probably now fewer such files in the wild).
Gale

Post by Paul Licameli
I discovered only today that fixes I made for memory leaks may have
invalidated the fix for the old, P5 bug, 451
http://bugzilla.audacityteam.org/show_bug.cgi?id=451#c16.
The problem was that when a project fails to open with parse errors, then
good block files were erased from disk causing data loss.
The original fix, I believe, spared the files but at the expense of

memory

Post by Paul Licameli
leaks.
I wrote an easy alternative fix that still spares the files and does not
leak memory, by doing something similar to what happens when the project
closes normally.
I have not yet tried to reproduce the problem and verify my fix. It

depends

Post by Paul Licameli
on invalid .aup fils after all.
I will make a branch called bug451 in my fork.
PRL
------------------------------------------------------------

------------------

Post by Paul Licameli
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today.http://sdm.link/intel
_______________________________________________
audacity-devel mailing list
https://lists.sourceforge.net/lists/listinfo/audacity-devel

------------------------------------------------------------
------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today.http://sdm.link/intel
_______________________________________________
audacity-devel mailing list
https://lists.sourceforge.net/lists/listinfo/audacity-devel

James Crook

2017-02-23 19:32:10 UTC

Permalink

Hi Paul,

Could you give a bit more detail, so that I can assess the risk?
Which memory-leak-fix commit reintroduced the issue with 451?

What I would need to check is whether 'more reasonable' ways of getting
bad projects (than using unicode/ANSI on 1.3.13) could trigger data
loss. For example, could truncation of the aup file lead to destruction
of (useful) block files before the user has had a chance to recover a
project?

--James.

Post by Paul Licameli
two months ago, for reopened bug 451, was never merged. So I am reminding
team about it again. It is a possible data loss bug that I re-introduced,
incidentally to the memory managment fixes. But Gale has said he thinks
the likelihood of this occurrence is much less than it was when the bug was
filed.
I should have checked up on this before RC2 was made, sorry. Do we want to
put this in?
PRL

memory

depends

Post by Paul Licameli
on invalid .aup fils after all.
I will make a branch called bug451 in my fork.
PRL

Gale Andrews

2017-02-23 21:04:32 UTC

Permalink

In the attachment to bug 451:
http://bugzilla.audacityteam.org/attachment.cgi?id=203

"Kevin_Test\Kevin_Test\Stage 2\Test2.aup"

when opened in HEAD, empties the _data folder.

So does Stage 3\Test2,aup.

This does not happen in 2.1.2.

No log message is shown in HEAD apart from:

Error: Could not parse file
"S:\Kevin_Test\Kevin_Test\Kevin_Test\Stage 2\Test2.aup".

Error: Error: not well-formed (invalid token) at line 26

(which is the same error given in 2.1.2).

Martyn's attachment to bug 451 does not create a problem AFAICT,

Gale

Post by James Crook
Hi Paul,
Could you give a bit more detail, so that I can assess the risk?
Which memory-leak-fix commit reintroduced the issue with 451?
What I would need to check is whether 'more reasonable' ways of getting
bad projects (than using unicode/ANSI on 1.3.13) could trigger data
loss. For example, could truncation of the aup file lead to destruction
of (useful) block files before the user has had a chance to recover a
project?
--James.

memory

depends

Post by Paul Licameli
on invalid .aup fils after all.
I will make a branch called bug451 in my fork.
PRL

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
https://lists.sourceforge.net/lists/listinfo/audacity-devel

James Crook

2017-02-23 22:17:05 UTC

Permalink

The actual bug seems to be that ANY corrupt .aup file will lead to
deletion of all data files that were listed in the .aup file before the
point of corruption. So 451 is 'the wrong' title for this new bug.

I tested this by truncating the aup file for a valid 2.1.3 project.

I would think this is either a P2 (data loss) or P1 (being also a
regression on 2.1.2).
It turns a faulty/damaged .aup into an unrecoverable project.
Gale, can you decide on a rating for it in the next two days?

--James.

Post by Gale Andrews
http://bugzilla.audacityteam.org/attachment.cgi?id=203
"Kevin_Test\Kevin_Test\Stage 2\Test2.aup"
when opened in HEAD, empties the _data folder.
So does Stage 3\Test2,aup.
This does not happen in 2.1.2.
Error: Could not parse file
"S:\Kevin_Test\Kevin_Test\Kevin_Test\Stage 2\Test2.aup".
Error: Error: not well-formed (invalid token) at line 26
(which is the same error given in 2.1.2).
Martyn's attachment to bug 451 does not create a problem AFAICT,
Gale

memory

depends

Post by Paul Licameli
on invalid .aup fils after all.
I will make a branch called bug451 in my fork.
PRL

Gale Andrews

2017-02-24 06:59:07 UTC

Permalink

Post by James Crook
The actual bug seems to be that ANY corrupt .aup file will lead to
deletion of all data files that were listed in the .aup file before the
point of corruption. So 451 is 'the wrong' title for this new bug.
I tested this by truncating the aup file for a valid 2.1.3 project.

So, just miss ">" off the final </project> and all the data is gone.

Testing it, it was this bad before e.g. in 1.3.13, before the bug 451 fix.

So not new.

Fortunately a corrupt AUTOSAVE file (binary) does not seem to lose
data.

Fortunately also, the quite common case where the AUP is empty
on save (all zeros) does not seem to cause loss of the data files.
Audacity just says it cannot import the AUP file.

Post by James Crook
I would think this is either a P2 (data loss) or P1 (being also a
regression on 2.1.2).
It turns a faulty/damaged .aup into an unrecoverable project.
Gale, can you decide on a rating for it in the next two days?

Can't decide right now. It only needs one thing that is not user
error to make me say P1. For example, emoji in the metadata
tags or file name which will give invalid token error. There is a
recent example on the Forum but I can't look now.

If you can prove that invalid token will now cause data loss, then
I'd say P1.

Gale

Post by James Crook

memory

depends

Post by Paul Licameli
on invalid .aup fils after all.
I will make a branch called bug451 in my fork.
PRL

Gale Andrews

2017-02-24 12:09:46 UTC

Permalink

To clarify, I regard it as P2 even if no further ways to trigger it
are found. So decision made, if James would block on it as P2.

Gale

Post by Gale Andrews

So, just miss ">" off the final </project> and all the data is gone.
Testing it, it was this bad before e.g. in 1.3.13, before the bug 451 fix.
So not new.
Fortunately a corrupt AUTOSAVE file (binary) does not seem to lose
data.
Fortunately also, the quite common case where the AUP is empty
on save (all zeros) does not seem to cause loss of the data files.
Audacity just says it cannot import the AUP file.

Can't decide right now. It only needs one thing that is not user
error to make me say P1. For example, emoji in the metadata
tags or file name which will give invalid token error. There is a
recent example on the Forum but I can't look now.
If you can prove that invalid token will now cause data loss, then
I'd say P1.
Gale

Post by James Crook

memory

depends

Post by Paul Licameli
on invalid .aup fils after all.
I will make a branch called bug451 in my fork.
PRL

Gale Andrews

2017-02-24 15:37:14 UTC

Permalink

Yes, if bad emoji characters are in the name of the second track of two,
then 2.1.3 empties the entire _data folder on opening the AUP file.

2.1.2 does not.

So it would have been P1 anyway.

Gale

Post by Gale Andrews

So, just miss ">" off the final </project> and all the data is gone.
Testing it, it was this bad before e.g. in 1.3.13, before the bug 451 fix.
So not new.
Fortunately a corrupt AUTOSAVE file (binary) does not seem to lose
data.
Fortunately also, the quite common case where the AUP is empty
on save (all zeros) does not seem to cause loss of the data files.
Audacity just says it cannot import the AUP file.

Can't decide right now. It only needs one thing that is not user
error to make me say P1. For example, emoji in the metadata
tags or file name which will give invalid token error. There is a
recent example on the Forum but I can't look now.
If you can prove that invalid token will now cause data loss, then
I'd say P1.
Gale

Post by James Crook

memory

depends

Post by Paul Licameli
on invalid .aup fils after all.
I will make a branch called bug451 in my fork.
PRL