Re: [Musicpd-dev-team] [PATCH] Add tag support to DSD decoders - v6

2012-08-21 Thread Jurgen Kramer
On Mon, 2012-08-20 at 08:44 +0200, Max Kellermann wrote:
 On 2012/08/16 18:18, Jurgen Kramer gtmkra...@xs4all.nl wrote:
  Lots of activity in git. Just a reminder for above commit.
 
 Memory leak in dsdlib_tag_id3().
Fixed. Added id3_tag_delete

 Can you use dsdlib_tag_id3() for a DoS attack?  This looks like it
 could easily cause a stack overflow:
 
 +   count = is-size - is-offset;
 +   id3_byte_t dsdid3[count];

What is your concern here? The allocation of dsdid3 using count or the
value of count being calculated that way (or both)?

 This looks suspicious, too:
 
 +   uint64_t length = (uint64_t)GUINT32_FROM_BE(metatag.size);
 +   char string[length];
 Why is this 32 bit integer casted to 64 bit and then back to 32 bit,
 anyway?
Changed that, length still needs conversion from uint32_t to size_t for usage 
in dsdlib_read unfortunately.

 Remember: bad files must *never* cause MPD to crash!  If my theory is
 true, this is a serious security vulnerability you're about to add to
 MPD.
I already have a version laying around where a put in some tighter checks on 
files being good DSF or DSDIFF files.
Maybe I'll sent in a patch for that first.

 scan_id3_tag() has duplicate documentation.  API documentation belongs
 to the header file.
OK, removed comments from .c file

 Remove the version number from the commit message.  I will not merge
 the old versions of your patch, these should be deleted.  Rebase on my
 master branch, and submit *only* the patches you want me to merge, not
 all the old versions.
I'll remove the version from the patch. 
Regarding only submitting the needed patches. What the commit I supplied
(http://git.musicpd.org/cgit/gtmkramer/mpd.git/commit/?id=9696eff5f075180e88fca504f2502c4f12aef71b)
you get more then needed? (I am still not fully proficient with git).

 
 Some API documentation for variables like diar_offset, diti_size could
 be useful.  I don't understand what they mean.
 
I'll add comments to the struct.

Jurgen


--
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
___
Musicpd-dev-team mailing list
Musicpd-dev-team@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team


Re: [Musicpd-dev-team] [PATCH] Add tag support to DSD decoders - v6

2012-08-21 Thread Jonathan Neuschäfer
On Tue, Aug 21, 2012 at 08:01:07PM +0200, Jurgen Kramer wrote:
 Regarding only submitting the needed patches. What the commit I supplied
 (http://git.musicpd.org/cgit/gtmkramer/mpd.git/commit/?id=9696eff5f075180e88fca504f2502c4f12aef71b)
 you get more then needed? (I am still not fully proficient with git).

Try git rebase -i HEAD~9 on (a copy of) that branch. That will allow
you to do several different things with the history in that branch.

StGit (stacked git) is another useful tool for managing patchsets in
git. (Its usage isn't overly obvious, but it has a good tutorial.)

hope that helps,
Jonathan Neuschäfer

--
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
___
Musicpd-dev-team mailing list
Musicpd-dev-team@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/musicpd-dev-team