Hi,
in one of our internal applications we are using SoX (the 14.4.2+git20190427
version from Ubuntu) to convert from a variety of audio formats to the WAV file
format. We observed that the tests for the conversion occasionally failed and
over the last days I found time to dig deeper into this.
We are using sox_open_memstream_write() to write to a dynamically allocated
in-memory stream. In our tests sometimes the size of the resulting WAV buffer
would have the expected size, sometimes it would be 44 bytes, the size of the
WAV header. Valgrind told me that the behavior of is_seekable() in formats.c
depends on uninitialized memory. In your git repository I found a fix for this:
commit bb38934e11035c8fab141f70dabda3afdd17da36
Author: Mans Rullgard <m...@mansr.com>
Date: Tue Aug 4 17:19:49 2020 +0100
format: improve is_seekable() test
Streams opened with fmemopen() do not have an underlying file descriptor,
so the fstat() will fail, and a random result is returned.
A simpler method that works regardless of file type is to call fseek()
and check if it reports success.
Suggested by Stefan Sauer <enso...@google.com>.
Now with this fix applied valgrind was happy, however now our conversion from
MP3 to WAV would always result in only 44 bytes, as read from the
buffer_size_ptr location passed to sox_open_memstream_write(). It turns out
that with above change the undefined behavior is fixed for streams created with
open_memstream() and is_seekable() will now reliably returns sox_true for such
streams. This allows the WAV writer code to do an fseek() to the start of the
stream followed by a write of the WAV header with correct length information.
However such a seek followed by a write causes the dynamically allocated memory
stream to be truncated. Thus after calling sox_close() the size reported for
the stream will be 44 bytes, that's not what we want. Unfortunately we can not
simply fix this by reporting the full buffer size as the buffer will actually
have been truncated, and a trailing null byte is appended after the WAV header.
It looks like we can indeed not seek and fix data in a dynamically allocated
stream. Thus I am attaching a patch that changes the code in formats.c to set
ft->seekable to false for streams opened with open_memstream(). With this
change applied on top of the improvement for the is_seekable() test, our tests
pass reliably and valgrind seems happy as well.
I am attaching the patch here, please consider it for inclusion. I am also
attaching a simple test application that writes to a stream, seeks to the front
and performs another write. The output of this program illustrates that the
buffer is truncated:
buf = `hello', size = 5
buf = `hello, world', size = 12
buf = `heyho', size = 5
Regards,
Sven
From 9a90484d6c7e23ce709e5e34eec2aec62b6d4cbc Mon Sep 17 00:00:00 2001
From: Sven Neumann <sven.neum...@logmein.com>
Date: Wed, 6 Oct 2021 17:36:26 +0200
Subject: [PATCH] formats: disallow seeking in dynamic memory buffers
Seeking in a dynamic memory buffer stream as provided by
open_memstream() truncates the memory buffer. Seeking back to
the start of the file to write a header will leave the user
with just the header then.
---
src/formats.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/formats.c b/src/formats.c
index 3fcf4382..45ca79ca 100644
--- a/src/formats.c
+++ b/src/formats.c
@@ -932,7 +932,8 @@ static sox_format_t * open_write(
lsx_fail("Can't set write buffer");
goto error;
}
- ft->seekable = is_seekable(ft);
+ /* Do not allow seeking in dynamic memory buffers as that would truncate the buffer. */
+ ft->seekable = (buffer_ptr && !buffer) ? sox_false : is_seekable(ft);
}
ft->filetype = lsx_strdup(filetype);
--
2.25.1
#include <stdio.h>
int
main (void)
{
char *bp;
size_t size;
FILE *stream;
stream = open_memstream (&bp, &size);
fprintf (stream, "hello");
fflush (stream);
printf ("buf = `%s', size = %ld\n", bp, size);
fprintf (stream, ", world");
fflush (stream);
printf ("buf = `%s', size = %ld\n", bp, size);
fseek (stream, 0, SEEK_SET);
fprintf (stream, "heyho");
fclose (stream);
printf ("buf = `%s', size = %ld\n", bp, size);
return 0;
}
_______________________________________________
SoX-devel mailing list
SoX-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sox-devel
