NCrusher74 / SwiftTaggerID3

A Swift library for reading and editing ID3 tags
Apache License 2.0
5 stars 2 forks source link

Crash on master branch #20

Closed myin4 closed 2 years ago

myin4 commented 3 years ago

hi @NCrusher74

I did check out the master branch (which recent you did merge the Unicode support from encoding-fixes branch) and run our projects with these songs attached here: NEW FOLDER.zip

Then there are crashes that come from SwiftTaggerID3 library. Here is the crash log:

Screen Shot 2021-04-07 at 05 48 23
myin4 commented 3 years ago

@NCrusher74 could you please help to check out what happened with the master branch?

Thanks and Bests Peter

NCrusher74 commented 3 years ago

Hi Peter.

I suspect what is happening is that, because we're now using UTF-16 encoding when attempting to encode to ISO-8859-1 fails, there is somewhere where the size of the frame isn't being counted correctly.

I will attempt to figure out where that is, but it would help if you could tell me what is happening earlier in the call stack. Which frame was it trying to read or encode when you received this error?

NCrusher74 commented 3 years ago

Hi Peter.

Upon further investigation, I noticed that when I attempted to read your sample files, the error for an invalid image format was being thrown (this happens when the artwork is not jpg or png.

Since your files definitely had jpg cover art, the problem had to be in the way SwiftTaggerID3 was attempting to read that frame.

The documentation on the ID3 spec is a little vague, so when I implemented the fixes to your last issue, I wasn't sure whether or not the string for the image format should be encoded as ASCII or encoded using the encoding applied to the other strings in the frame (such as the image description).

I defaulted to using the encoding applied to the user-defined strings, and apparently this was the wrong approach. If the image description it would be fine if the mp3 file were actually written by SwiftTaggerID3, but if that isn't the case, a situation where the frame is being read with SwiftTaggerID3 expecting more bytes in the frame (and that string, in particular) than are actually there.

That would both result in an UnhandledImageFormat error being thrown upon attempt to read this frame, and could also create problems down the line the the expected size of the tag as a whole being incorrect. Which is, I suspect, the problem you encountered, though I have no way of knowing for sure without more information.

I have made a change so that this image format string is encoded as ASCII (there is no reason why the image format string should ever NOT be ASCII, since it's not a user-defined string) and now your sample files are being read correctly.

I would appreciate it if you would try using the imageFrameError branch and seeing if that resolves your issue.

If not, there may be other places where I erred on the side of encoding a string that should only be ASCII or ISO-8859-1 using the encoding applied to user-defined strings, and I'll have to hunt those down.

Thank you.

myin4 commented 3 years ago

Hi Peter.

I suspect what is happening is that, because we're now using UTF-16 encoding when attempting to encode to ISO-8859-1 fails, there is somewhere where the size of the frame isn't being counted correctly.

I will attempt to figure out where that is, but it would help if you could tell me what is happening earlier in the call stack. Which frame was it trying to read or encode when you received this error?

Hi @NCrusher74 Thank you for your quick response. I appreciated!

Here is the call stack: Screen Shot 2021-04-07 at 17 34 47

myin4 commented 3 years ago

Hi Peter.

Upon further investigation, I noticed that when I attempted to read your sample files, the error for an invalid image format was being thrown (this happens when the artwork is not jpg or png.

Since your files definitely had jpg cover art, the problem had to be in the way SwiftTaggerID3 was attempting to read that frame.

The documentation on the ID3 spec is a little vague, so when I implemented the fixes to your last issue, I wasn't sure whether or not the string for the image format should be encoded as ASCII or encoded using the encoding applied to the other strings in the frame (such as the image description).

I defaulted to using the encoding applied to the user-defined strings, and apparently this was the wrong approach. If the image description it would be fine if the mp3 file were actually written by SwiftTaggerID3, but if that isn't the case, a situation where the frame is being read with SwiftTaggerID3 expecting more bytes in the frame (and that string, in particular) than are actually there.

That would both result in an UnhandledImageFormat error being thrown upon attempt to read this frame, and could also create problems down the line the the expected size of the tag as a whole being incorrect. Which is, I suspect, the problem you encountered, though I have no way of knowing for sure without more information.

I have made a change so that this image format string is encoded as ASCII (there is no reason why the image format string should ever NOT be ASCII, since it's not a user-defined string) and now your sample files are being read correctly.

I would appreciate it if you would try using the imageFrameError branch and seeing if that resolves your issue.

If not, there may be other places where I erred on the side of encoding a string that should only be ASCII or ISO-8859-1 using the encoding applied to user-defined strings, and I'll have to hunt those down.

Thank you.

Hi @NCrusher74

Thank you for your supporting

I did try imageFrameError branch. Here is what happen:

  1. Crash is gone.
  2. No more crash with Unicode strings
  3. But I can not set frontCover of the mp3 files anymore (it worked before). I am call this function to set frontCover: try newTag.set(attachedPicture: .frontCover, imageLocation: URL(fileURLWithPath: urlPath!), description: "")

Could you please help to fix it?

Thank you so much for your support!

Thanks and Bests, Peter

NCrusher74 commented 3 years ago

Hi Peter. Sorry about that, I forgot the image format string was supposed to be null-terminated in versions 2.3/2.4. It should work now. Pull the branch and try again, and if it works for your testing, I'll merge it.

myin4 commented 3 years ago

Thank you so much @NCrusher74 It worked. Could you please help to merge to the master branch?

Peter Thanks and Bests

NCrusher74 commented 3 years ago

Changes are merged and the master branch should be good to go. Good luck!

myin4 commented 3 years ago

Hello @NCrusher74

Thank you for your quick response and quick fix.

Currently, I have 3 mp3 files which contain the Unicode in some fields and it makes your library crash when init the Tag object. You can try these 3 mp3 files from this downloadable link: https://drive.google.com/file/d/113igerzgLBekw85rhIKNVFTShBmhF1hn/view?usp=sharing

Could you please help to support Unicode for all ID3 fields?

Again thank you so much for your quick support!

Peter, Thanks and Bests

NCrusher74 commented 3 years ago

Hi Peter. I'm on my way out the door in a moment and will be out of town for a couple days. I will get back to you the day after tomorrow, unless I have some downtime to check things out at my hotel. Thank you.

NCrusher74 commented 3 years ago

Hi Peter. The only problem I could find with any of your files was that two of them had .bmp cover art instead of .jpg or .png. I hadn't included handling for .bmp in SwiftTaggerID3 because the ID3 spec specifically says that jpg/png are preferable for interoperability.

However, since it has become an issue. I have expanded the handling of cover images to include .bmp, gif, and tiff. Because I don't have files using gif and tiff as cover art, however, I'm not able to test those.

As far as I can tell from my end, there is no problem with unicode in any of these files. The only problem was that they were throwing an "Unhandled Image Format" error until I included handling for more image formats.

The changes are in version 1.7.2. Update your dependencies and see if that solves your issue. Otherwise, I may need more information about the problem.

myin4 commented 3 years ago

Hi Peter. The only problem I could find with any of your files was that two of them had .bmp cover art instead of .jpg or .png. I hadn't included handling for .bmp in SwiftTaggerID3 because the ID3 spec specifically says that jpg/png are preferable for interoperability.

However, since it has become an issue. I have expanded the handling of cover images to include .bmp, gif, and tiff. Because I don't have files using gif and tiff as cover art, however, I'm not able to test those.

As far as I can tell from my end, there is no problem with Unicode in any of these files. The only problem was that they were throwing an "Unhandled Image Format" error until I included handling for more image formats.

The changes are in version 1.7.2. Update your dependencies and see if that solves your issue. Otherwise, I may need more information about the problem.

Hi @NCrusher74

I am going to integrate version 1.7.2 into our project and test it. Thank you for your support.

Bests And Regards, Peter