[FFmpeg-devel] [PATCH] avformat/id3v2: support buggy id3v2.3 tag length in id3v2.4

2014-10-17 Thread Benoit Fouet 
Some encoders do not use syncsafe sizes in v2.4 id3 tags. Check the next
tag to try to choose between the two.

Fixes ticket #4003
---
 libavformat/id3v2.c | 64 +++--
 1 file changed, 62 insertions(+), 2 deletions(-)

diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
index 44df4ff..3e54620 100644
--- a/libavformat/id3v2.c
+++ b/libavformat/id3v2.c
@@ -170,6 +170,48 @@ static unsigned int get_size(AVIOContext *s, int len)
 return v;
 }
 
+static unsigned int size_to_syncsafe(unsigned int size)
+{
+return (((size)  (0x7f   0))  0) +
+   (((size)  (0x7f   8))  1) +
+   (((size)  (0x7f  16))  2) +
+   (((size)  (0x7f  24))  3);
+}
+
+/* No real verification, only check that the tag consists of
+ * a combination of capital alpha-numerical characters */
+static int is_tag(const char *buf, unsigned int len)
+{
+if (!len)
+return 0;
+
+while (len--)
+if ((buf[len]  'A' ||
+ buf[len]  'Z') 
+(buf[len]  '0' ||
+ buf[len]  '9'))
+return 0;
+
+return 1;
+}
+
+/**
+ * Return 1 if the tag of length len at the given offset is valid, 0 if not, 
-1 on error
+ */
+static int check_tag(AVIOContext *s, int offset, unsigned int len)
+{
+char tag[4];
+
+if (len  4 ||
+avio_seek(s, offset, SEEK_SET)  0 ||
+avio_read(s, tag, len)  len)
+return -1;
+else if (!AV_RB32(tag) || is_tag(tag, len))
+return 1;
+
+return 0;
+}
+
 /**
  * Free GEOB type extra metadata.
  */
@@ -734,8 +776,26 @@ static void id3v2_parse(AVIOContext *pb, AVDictionary 
**metadata,
 tag[4] = 0;
 if (version == 3) {
 tlen = avio_rb32(pb);
-} else
-tlen = get_size(pb, 4);
+} else {
+/* some encoders incorrectly uses v3 sizes instead of syncsafe 
ones
+ * so check the next tag to see which one to use */
+tlen = avio_rb32(pb);
+if (tlen  0x7f) {
+if (tlen  len) {
+int64_t cur = avio_tell(pb);
+
+if (ffio_ensure_seekback(pb, 2 /* tflags */ + tlen + 4 
/* next tag */))
+break;
+
+if (check_tag(pb, cur + 2 + size_to_syncsafe(tlen), 4) 
== 1)
+tlen = size_to_syncsafe(tlen);
+else if (check_tag(pb, cur + 2 + tlen, 4) != 1)
+break;
+avio_seek(pb, cur, SEEK_SET);
+} else
+tlen = size_to_syncsafe(tlen);
+}
+}
 tflags  = avio_rb16(pb);
 tunsync = tflags  ID3v2_FLAG_UNSYNCH;
 } else {
-- 
2.1.2.443.g670a3c1

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/id3v2: support buggy id3v2.3 tag length in id3v2.4

2014-10-17 Thread Michael Niedermayer 
On Fri, Oct 17, 2014 at 10:56:59AM +0200, Benoit Fouet wrote:
 Some encoders do not use syncsafe sizes in v2.4 id3 tags. Check the next
 tag to try to choose between the two.
 
 Fixes ticket #4003
 ---
  libavformat/id3v2.c | 64 
 +++--
  1 file changed, 62 insertions(+), 2 deletions(-)

applied

thanks

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] avformat/id3v2: support buggy id3v2.3 tag length in id3v2.4

2014-10-16 Thread Benoit Fouet 
Some encoders do not use syncsafe sizes in v2.4 id3 tags. Check the next
tag to try to choose between the two.

Fixes ticket #4003
---
 libavformat/id3v2.c | 42 +-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
index 5469e0a..3bccd76 100644
--- a/libavformat/id3v2.c
+++ b/libavformat/id3v2.c
@@ -170,6 +170,23 @@ static unsigned int get_size(AVIOContext *s, int len)
 return v;
 }
 
+/* No real verification, only check that the tag consists of
+ * a combination of capital alpha-numerical characters */
+static int is_tag(const char *buf, int len)
+{
+if (!len)
+return 0;
+
+while (len--)
+if ((buf[len]  'A' ||
+ buf[len]  'Z') 
+(buf[len]  '0' ||
+ buf[len]  '9'))
+return 0;
+
+return 1;
+}
+
 /**
  * Free GEOB type extra metadata.
  */
@@ -734,8 +751,31 @@ static void id3v2_parse(AVIOContext *pb, AVDictionary 
**metadata,
 tag[4] = 0;
 if (version == 3) {
 tlen = avio_rb32(pb);
-} else
+} else {
 tlen = get_size(pb, 4);
+if (tlen  0x7f) {
+/* some encoders incorrectly uses v3 sizes instead of 
syncsafe ones
+ * so check the next tag to see which one to use */
+int64_t cur = avio_tell(pb);
+char next_tag[5];
+
+next = cur + 2 /* tflags */ + tlen;
+avio_seek(pb, next, SEEK_SET);
+if (avio_read(pb, next_tag, 4)  4)
+break;
+if (AV_RB32(next_tag)  !is_tag(next_tag, 4)) {
+avio_seek(pb, cur - 4, SEEK_SET);
+tlen = avio_rb32(pb);
+next = cur + 2 + tlen;
+avio_seek(pb, next, SEEK_SET);
+if (avio_read(pb, next_tag, 4)  4)
+break;
+if (AV_RB32(next_tag)  !is_tag(next_tag, 4))
+break;
+}
+avio_seek(pb, cur, SEEK_SET);
+}
+}
 tflags  = avio_rb16(pb);
 tunsync = tflags  ID3v2_FLAG_UNSYNCH;
 } else {
-- 
2.1.2.443.g670a3c1

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/id3v2: support buggy id3v2.3 tag length in id3v2.4

2014-10-16 Thread Michael Niedermayer 
On Thu, Oct 16, 2014 at 11:44:47AM +0200, Benoit Fouet wrote:
 Some encoders do not use syncsafe sizes in v2.4 id3 tags. Check the next
 tag to try to choose between the two.
 
 Fixes ticket #4003
 ---
  libavformat/id3v2.c | 42 +-
  1 file changed, 41 insertions(+), 1 deletion(-)
 
 diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
 index 5469e0a..3bccd76 100644
 --- a/libavformat/id3v2.c
 +++ b/libavformat/id3v2.c
 @@ -170,6 +170,23 @@ static unsigned int get_size(AVIOContext *s, int len)
  return v;
  }
  
 +/* No real verification, only check that the tag consists of
 + * a combination of capital alpha-numerical characters */
 +static int is_tag(const char *buf, int len)
 +{
 +if (!len)
 +return 0;
 +
 +while (len--)
 +if ((buf[len]  'A' ||
 + buf[len]  'Z') 
 +(buf[len]  '0' ||
 + buf[len]  '9'))
 +return 0;
 +
 +return 1;
 +}
 +
  /**
   * Free GEOB type extra metadata.
   */
 @@ -734,8 +751,31 @@ static void id3v2_parse(AVIOContext *pb, AVDictionary 
 **metadata,
  tag[4] = 0;
  if (version == 3) {
  tlen = avio_rb32(pb);
 -} else
 +} else {
  tlen = get_size(pb, 4);
 +if (tlen  0x7f) {

i think that check isnt sufficient to detect that the 2 readings
differ. For example 0xFF would be read as 0xFF by one and 0x7F by
the other and would not trigger this

also maybe len could be used to distingish a subset of cases

and there is the problem with seekable=0 streams where seeking
back might fail, not sure what to do in that case ...


[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

He who knows, does not speak. He who speaks, does not know. -- Lao Tsu


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/id3v2: support buggy id3v2.3 tag length in id3v2.4

2014-10-16 Thread Benoit Fouet 
Hi,

On 16 October 2014 17:10:38 CEST, Michael Niedermayer michae...@gmx.at wrote:
On Thu, Oct 16, 2014 at 11:44:47AM +0200, Benoit Fouet wrote:
 Some encoders do not use syncsafe sizes in v2.4 id3 tags. Check the
next
 tag to try to choose between the two.
 
 Fixes ticket #4003
 ---
  libavformat/id3v2.c | 42 +-
  1 file changed, 41 insertions(+), 1 deletion(-)
 
 diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
 index 5469e0a..3bccd76 100644
 --- a/libavformat/id3v2.c
 +++ b/libavformat/id3v2.c
 @@ -170,6 +170,23 @@ static unsigned int get_size(AVIOContext *s, int
len)
  return v;
  }
  
 +/* No real verification, only check that the tag consists of
 + * a combination of capital alpha-numerical characters */
 +static int is_tag(const char *buf, int len)
 +{
 +if (!len)
 +return 0;
 +
 +while (len--)
 +if ((buf[len]  'A' ||
 + buf[len]  'Z') 
 +(buf[len]  '0' ||
 + buf[len]  '9'))
 +return 0;
 +
 +return 1;
 +}
 +
  /**
   * Free GEOB type extra metadata.
   */
 @@ -734,8 +751,31 @@ static void id3v2_parse(AVIOContext *pb,
AVDictionary **metadata,
  tag[4] = 0;
  if (version == 3) {
  tlen = avio_rb32(pb);
 -} else
 +} else {
  tlen = get_size(pb, 4);
 +if (tlen  0x7f) {

i think that check isnt sufficient to detect that the 2 readings
differ. For example 0xFF would be read as 0xFF by one and 0x7F by
the other and would not trigger this
 

True, although I guess that could be handled by just making this test a = 
instead of .

also maybe len could be used to distingish a subset of cases
 

Not sure I understand what you mean here... The tlen check seems to be enough 
to me.

and there is the problem with seekable=0 streams where seeking
back might fail, not sure what to do in that case ...
 

We could theoretically buffer the data instead of seeking back, as the syncsafe 
size will always be smaller than the other one. Is this something that we want?
I can work on something like that.

-- 
Ben


___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/id3v2: support buggy id3v2.3 tag length in id3v2.4

2014-10-16 Thread Michael Niedermayer 
On Thu, Oct 16, 2014 at 06:06:39PM +0200, Benoit Fouet wrote:
 Hi,
 
 On 16 October 2014 17:10:38 CEST, Michael Niedermayer michae...@gmx.at 
 wrote:
 On Thu, Oct 16, 2014 at 11:44:47AM +0200, Benoit Fouet wrote:
  Some encoders do not use syncsafe sizes in v2.4 id3 tags. Check the
 next
  tag to try to choose between the two.
  
  Fixes ticket #4003
  ---
   libavformat/id3v2.c | 42 +-
   1 file changed, 41 insertions(+), 1 deletion(-)
  
  diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
  index 5469e0a..3bccd76 100644
  --- a/libavformat/id3v2.c
  +++ b/libavformat/id3v2.c
  @@ -170,6 +170,23 @@ static unsigned int get_size(AVIOContext *s, int
 len)
   return v;
   }
   
  +/* No real verification, only check that the tag consists of
  + * a combination of capital alpha-numerical characters */
  +static int is_tag(const char *buf, int len)
  +{
  +if (!len)
  +return 0;
  +
  +while (len--)
  +if ((buf[len]  'A' ||
  + buf[len]  'Z') 
  +(buf[len]  '0' ||
  + buf[len]  '9'))
  +return 0;
  +
  +return 1;
  +}
  +
   /**
* Free GEOB type extra metadata.
*/
  @@ -734,8 +751,31 @@ static void id3v2_parse(AVIOContext *pb,
 AVDictionary **metadata,
   tag[4] = 0;
   if (version == 3) {
   tlen = avio_rb32(pb);
  -} else
  +} else {
   tlen = get_size(pb, 4);
  +if (tlen  0x7f) {
 
 i think that check isnt sufficient to detect that the 2 readings
 differ. For example 0xFF would be read as 0xFF by one and 0x7F by
 the other and would not trigger this
  
 
 True, although I guess that could be handled by just making this test a = 
 instead of .

that wouldnt work with 0x81


 
 also maybe len could be used to distingish a subset of cases
  
 
 Not sure I understand what you mean here... The tlen check seems to be enough 
 to me.

i was thinking of avoiding the seek for cases where one is clearly
invalid like tlen  len IIRC the variable names


 
 and there is the problem with seekable=0 streams where seeking
 back might fail, not sure what to do in that case ...
  
 
 We could theoretically buffer the data instead of seeking back, as the 
 syncsafe size will always be smaller than the other one. Is this something 
 that we want?
 I can work on something like that.

probably that is needed, also see ffio_ensure_seekback()

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel