[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#890023: abcmidi fails autopkg tests on 32bit architectures



Control: tags -1 patch

Hi,

On 10/02/18 08:52, Matthias Klose wrote:
> Package: src:abcmidi
> Version: 20180125-1
> Severity: serious
> Tags: sid buster
> 
> abcmidi fails autopkg tests on 32bit architectures, midi2abc crashing.  Is there
> a reason why the tests are not run at build time?
> 
> --------------------------------
> MIDI to ABC conversion test
> --------------------------------
> Convert the araber47.mid file back to abc
> Aborted (core dumped)
> autopkgtest [14:08:29]: test conversions: -----------------------]
> autopkgtest [14:08:33]: test conversions:  - - - - - - - - - - results - - - - -
> - - - - -
> conversions          FAIL non-zero exit status 134
> autopkgtest [14:08:33]: test conversions:  - - - - - - - - - - stderr - - - - -
> - - - - -
> *** Error in `midi2abc': free(): invalid pointer: 0x00c43f28 ***
> Aborted (core dumped)

Caused by a buffer overflow in midi2abc.c:329
> char* addstring(s)
> /* create space for string and store it in memory */
> char* s;
> {
>   char* p;
> 
>   p = (char*) checkmalloc(strlen(s)+1);
>   strncpy(p, s,strlen(s)+2); /* [SS] 2017-08-30 */
>   return(p);
> }

strncpy writes to exactly n bytes to the output buffer, so the call will
always overflow the buffer allocated one line above by 1 byte.

Attached patch fixes this. I think using strcpy is safe here because the
size of the buffer allocated is always greater than the string length.

I think the comment on that line refers to this (from doc/CHANGES):
> August 30 2017
> 
> Midi2abc - The metatext string is not terminated with a 0 and
> as a result can contain random junk, in particular on the Windows
> operating system. Fix in midifile.c, the Msgbuff is initialized to
> 0 when it is allocated.

Some old code I found shows the code originally used strcpy. I'm not I
understand how using strncpy was supposed to fix this. I've copied
upstream who might be able to shed some light on this.

Thanks,
James
--- a/midi2abc.c
+++ b/midi2abc.c
@@ -333,7 +333,7 @@ char* s;
   char* p;
 
   p = (char*) checkmalloc(strlen(s)+1);
-  strncpy(p, s,strlen(s)+2); /* [SS] 2017-08-30 */
+  strcpy(p, s);
   return(p);
 }
 

Attachment: signature.asc
Description: OpenPGP digital signature


Reply to: