Bug#765009: Subject: RFS: abcmidi/20140928-1 [ITA]

[Tobias Frost]

Many thanks James! Valid points 

Ross, please also consider those comments. Especially please fix the
build system. I missed that during my review, sorry, but I will file a
bug for that.

(Also, please send your patches upstream.)

--
tobi


Am Montag, den 20.10.2014, 21:59 +0100 schrieb James Cowgill:
> On Mon, 2014-10-20 at 15:59 +0200, Ross Gammon wrote:
> > Hi All,
> > 
> > I know everyone is busy with the Jessie Release Freeze, but I would be
> > grateful if somebody could take a look at abcmidi (and sponsor if
> > happy). Abcmidi has been sitting unloved for a while now (since 2007).
> > It would be great to get the latest version into Jessie.
> 
> Hi,
> 
> Here's a review (I'm not a DD so can't sponsor you however).
> 
> General
> * There is a new upstream version (16th October 2014).
> * #764998 abcmidi: binary-without-manpage usr/bin/abcmatch
>   Obviously you know this, but it would be good if a manpage was added.
> * The file "/usr/share/doc/abcmidi/VERSION" seems redundant and can
>   probably be removed.

Also ÁUTHORS should not be installed.


> debian/copyright
> * You don't need to list abc.h, sizes.h, structs.h manually in the first
>   section since they're already included when you say "Files: *".
> * There seems to be some confusion about whether the code is GPL-2 or
>   GPL-2+. Are you sure what you've put is correct? I see files with no
>   copyright headers but nothing with "GPL 2 only" in them.
> * You don't need to repeat the GPL header lots of times. I'd also be
>   tempted to merge all the GPL sections together and just have a large
>   "Copyright:" block.
> 
> debian/rules
> * I don't think you need to use autotools-dev in this package (I don't
>   know a huge amount about this though).
> * The clean target doesn't work because you disabled it. This is a
>   violation of debian policy (4.9) "clean (required): This must undo any
>   effects that the build and binary targets may have had"
> 
> debian/patches:
> * Make sure you send these patches upstream (sorry if you've already 
>   done this - they're not in the new version though).
> * hardening.patch: Only LDFLAGS should be passed during the link stage. 
>   Remove your CFLAGS and CPPFLAGS additions.
> 
> Build
> There are lots of bad warnings printed when building this
> Examples:
> * parseabc.c:1701:3: warning: format ‘%s’ expects argument of type ‘char *’, but argument 3 has type ‘char **’ [-Wformat=]
>    success = sscanf (s, "%%abc-version %s", &abcversion); /* [SS] 2014-08-11 */
>  
>   Isn't this a buffer overflow?!
> 
> * toabc.c:1490:8: warning: iteration 7u invokes undefined behavior [-Waggressive-loop-optimizations]
>    semi = convertnote[i];
> 
> It's not too difficult to use these to make abc2midi segfault - please
> try and fix them if you have time.
> 
> James
> 
>