Bug #66768 for MP3-Tag: Broken temp file generation in MP3::Tag::ID3v2

On Tue, Mar 22, 2011 at 01:48:25AM -0400, Jason Rhinelander via RT wrote: Show quoted text
I won't like to introduce an extra dependency to fix a rare race condition. (Especially taking into account that `use File::Temp' pulls in 29 modules, including overload.pm!) If you are interested, consider some more lightweight solution... Checking... Well, it looks like File::Temp is included even in 5.6.1, so it won't be a problem if it were not so heavy-weight... Ilya

On 22-Mar-11 4:06 PM, Ilya Zakharevich via RT wrote: Show quoted text
That, of course, is exactly what File::Temp does, but it takes a fair chunk of code to get this right in a portable manner. The proper solution here is to use File::Temp--trying to duplicate everything it does, especially in dealing with odd systems, hardly seems worth the effort and the added maintenance cost just to avoid depending on a *core* Perl module that, honestly, doesn't have all that many dependencies. The -runtime patch I attached earlier won't even load File::Temp most of the time, avoiding any resource hit in the majority of cases (it only gets loaded if MP3::Tag::ID3v2 needs to rewrite the entire file). We're still talking a relatively small amount of system resources to load File::Temp and its dependencies, and it seems like reducing the loading to when we actually need it is worthwhile as it fixes a serious (i.e. data loss) problem in a known, correct, portable, tested way. I really want to ask that you reconsider the -runtime patch I sent in my last e-mail. Using File::Temp is the standard, accepted, core Perl way of avoiding exactly this problem, and if it's only loaded when rewriting an entire file (on the order of a few MB in typical use cases), the overhead of File::Temp seems worthwhile to avoid data loss. Jason

On 22-Mar-11 10:51 PM, Ilya Zakharevich via RT wrote: Show quoted text
Look for yourself. vim `perldoc -l File::Temp` Show quoted text
Why not? Jason

On Wed, Mar 23, 2011 at 12:34:51AM -0400, Jason Rhinelander via RT wrote: Show quoted text
Irrelevant. Loading overload.pm already does the damage... Yours, Ilya

On Thu, Mar 24, 2011 at 03:21:18PM -0400, Jason Rhinelander via RT wrote: Show quoted text
When I implemented overloading for Perl, I, obviously, did a lot of benchmarking. That time superpipelining and cache-coloring-type optimizations in the processors were not widespread, so one could do "pretty predictable" benchmarking. IIRC, the slowdown due to my patch was about +- 1% when overloading was not used (probably due to offset of addresses, so better or worse alignment of cache lines), and "about several percent" (do not remember more precise) when overload was used. On processors of nowadays, benchmarks vary too much to be meaningful without major statistical samples. Yours, Ilya

While I'm still not convinced that a performance deficit that requires "major statistical samples" to detect, which is only loaded for a small fraction of MP3::Tag's use case, I'm willing to concede the point in order to get this fixed in *some* way. So let me ask this: are you willing to apply some patch on this, and if so, what code is acceptable here? The code from perldoc -q temporary (modified as appropriate, of course--actually, at a quick glance that code appears wrong in that it tests *FH, but actually opens a lexical filehandle $fh)? File::Temp appears to have additional safeness code for systems where the simple sysopen with O_WRONLY|O_EXCL|O_CREAT is insufficient to guarantee safeness, but that code plus a randomly generated sequence (instead of the current counter-from-0 approach) ought to eliminate on POSIX-like systems on non-networked filesystems, and significantly reduce the occurrence otherwise. So in short, I suggest, and will write it and submit a patch, if you concur: - using dirname($filename) . "/TMP" . (random sequence of 5 alphanumeric characters) . ".tmp" for the filename - using sysopen() with O_WRONLY|O_EXCL|O_CREAT instead of open() Does that work? Jason On 24-Mar-11 5:05 PM, Ilya Zakharevich via RT wrote: Show quoted text

On 24-Mar-11 5:46 PM, Jason Rhinelander wrote: Show quoted text
... plus a bunch of extra tests if someone increases File::Temp's safe_level.

On 24-Mar-11 10:42 PM, Ilya Zakharevich via RT wrote: Show quoted text
I do not know. I would guess "probably" for O_BINARY and O_EXLOCK, and "maybe" for the others. I do, however, assume that the author(s) of File::Temp knows better than I, and so assume that they have some importance on some system somewhere. I've written another patch, addressing the various concerns you have raised in this report thus far. This time using sysopen, as previously discussed, with all the various flags that File::Temp sets. If you know any of these to be obviously unnecessary for the file replacement being done in MP3::Tag::ID3v2, please remove them. Additionally, this patch uses TMPxxxx.tmp for the filename, where xxxxx is replaced with 5 random \w characters (this ought to help when the filesystem is on some mountpoint (e.g. a network mount) where sysopen cannot guarantee safeness of the open). Finally, the patch also attempts to preserve the file permissions of the original file (though forcing 0600 at a minimum). I tested the patch on a few MP3s, completely removing and re-adding the ID3v2 tags to force the file rewriting, and everything appears to be working properly. I specifically, for testing, changed the ID3v2.pm code to use much less random filename generation with several calls in parallel, and verified that the sysopen call, in fact, failing correctly when the file already exists. Hopefully you find this new patch acceptable, and can include it in the next release of MP3::Tag. Thanks, Jason

On Fri, Mar 25, 2011 at 03:08:14AM -0400, Jason Rhinelander via RT wrote: Show quoted text
Show quoted text
Show quoted text
"Definitely NO" for O_BINARY and O_EXLOCK. Show quoted text
The only one which may have some vague relation to real needs is O_LARGEFILE. Unfortunately, I have no clue what it means.... Show quoted text
As I said, I do not give ANY trust to authors of File::Temp, judging by how it is designed... Show quoted text
Thanks! I was planning to do this for ages, just did not know how to do it exactly... Your prodding worked well in this regard... Show quoted text
I thought about this, and I'm not absolutely sure that it is what a user would want. Maybe one can make it configurable, as other hard-to-decide matters... Yours, Ilya

On 25-Mar-11 5:47 AM, Ilya Zakharevich via RT wrote: Show quoted text
Since guaranteed independence is not required here, that is not a bug, and you know it. Your hostility, however, tells me that I am wasting my time in attempting to propose a solution to this bug. You have my patches; use bits of them or do something else as you like. Jason

On 25/03/11 11:05 AM, Ilya Zakharevich via RT wrote: Show quoted text
I apologize for the tenor of my previous e-mail. I am interested in getting this fixed, as I find the module very useful, and do appreciate the desire to get it fixed correctly. Show quoted text
The bigger problem here is that even scooping the bits out of a single rand() call doesn't actually give enough randomness: in particular, if I fork (presuming rand() has been called at least once before the fork(), i.e. so that rand has been seeded), then attempt to operate on two files in the same directory at the same time, the same rand() value (and, for that matter, sequence of rand() values) will be generated--whether or not I use the bits from one rand() call or a sequence of rand() calls. Since this race condition seems most likely to come up in a script that forks and uses MP3::Tag in parallel (I may be biased--but that was how I ran into it), I tend to agree that any use of rand() here--whether 5 rand() calls, or one rand() call divided into 5 sets of bits--isn't actually any more random than the existing 0..999 sequence, and thus isn't solving anything when sysopen can't guarantee non-overwriting properly. I'm not entirely sure what a feasible solution is here. You mentioned earlier that there were portability problems basing the filename on the MP3 filename as I did in one of the earlier patches--could we amend that filename generation in some way to mitigate the problem here? On a separate note, while looking at the code I noticed another potential data loss problem: print values aren't checked, so if writing to the temp MP3 file fails (e.g. because of a full disk), we still clobber the original with a truncated version. If you'd like, I'd be happy to submit a separate patch that checks those, unlinking the tempfile and returning an error value if a print fails. I didn't want to throw it into the previous patch as it really is a separate issue--but the two patches touch some of the same code and would thus be dependent. Jason

On Fri, Mar 25, 2011 at 03:22:24PM -0400, Jason Rhinelander via RT wrote: Show quoted text
Untested: my $r = @arr ** 5; # limit # processes may have the same seed if rand() is called before fork() $r = ($$ + int(rand $r)) % $r; # read randomness from ONE call to rand() my $X = join '', map { my $o = $r % @arr; $r = int($r/@arr); chr $o } 1..5; Yours, Ilya

On Fri, Mar 25, 2011 at 03:22:24PM -0400, Jason Rhinelander via RT wrote: Show quoted text
Checking result of print() is not needed. But one MUST check the result of close() indeed! The only excuse I have is that "it is not my code", and I just have not have tuits to handle it... Show quoted text
Thanks a lot! However, note that I do not see how returning an error value would be a solution. I suspect one should better die() instead... Thanks, Ilya

On Mon, Mar 28, 2011 at 09:18:57AM -0400, Jason Rhinelander via RT wrote: Show quoted text
"Unable to rename it" is tricky: the user who reads error messages has a chance to correct it later. I would make it user-configurable with the default being "be fatal". Hmm, "unable to do something" is ALWAYS correctable later - unless we do something really stupid (like the current practice of overwriting files ;-). So the criterion should better be "is the error expected to persist if we do many files in a loop". "Unable to rename" looks like some file locking issue which MAY be specific to a particular file. The other two errors look like some filesystem problems, and would usually "persist" for other files as well... Just thinking aloud... So maybe "unable to rename it" reserves a special treatment indeed... Ilya

I should also add that I'm driven a little mad by the conflicting tab settings: the file has apparently been edited by people using a mixture of 4-space tabs, 8-space tabs, and pure space indentation, for which no tabstop setting seems to work across the whole file. I tried to keep the tabbing in each area the same so as to avoid unnecessary diff bloat, but reindenting the file in some universal style would be worthwhile. Jason