Bug#890023: abcmidi fails autopkg tests on 32bit architectures
Control: tags -1 pending
Hi James,
As usual, thanks for your help in tracking this down. I had noticed that
abcmidi was stuck in ubuntu -proposed, but I had not had time to
investigate.
As the changes seem to have been about fixing a windows problem, I am
sure the patch will be fine for Debian/Ubuntu. Seymour (upstream) will
no doubt consider applying the patch, or something better if it causes a
regression on Windows.
I hope to apply the patch in Debian, and upload with the latest upstream
release sometime this week.
Regards,
Ross
On 02/12/2018 02:28 PM, James Cowgill wrote:
> 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
Reply to: