sox-devel@lists.sourceforge.net unofficial mirror
 help / color / mirror / code / Atom feed
From: Sun Zhenliang <hisunzhenliang@outlook.com>
To: sox-devel@lists.sourceforge.net
Subject: Re: [PATCH] formats: disallow seeking in dynamic memory buffers
Date: Thu, 7 Oct 2021 10:24:21 +0800	[thread overview]
Message-ID: <OS3PR01MB5976D542EE0DAB4C146D0361C1B19@OS3PR01MB5976.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CO1PR15MB4890C2072EF9A67BB29DB6E08CB09@CO1PR15MB4890.namprd15.prod.outlook.com>


[-- Attachment #1.1: Type: text/plain, Size: 3912 bytes --]

在 2021年10月7日 +0800 05:35,Sven Neumann via SoX-devel <sox-devel@lists.sourceforge.net>,写道:
> 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 <mans@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 <ensonic@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
Bug mentioned https://sourceforge.net/p/sox/mailman/message/37325794/.

It is possible to fix the whole data. From my test to open_memstream(), the
dynamically allocated memory would not be free before the ft->fp was closed.
The data written is still there if it’s not covered by other write opt.

In this WAV issue, the seek opt is just in sox_close() to rewrite header, which
is the end of transcoding and will not cover the data area. We can just ftell()
to get the end of file before the fseek and seek back to the end after rewriting.

Output with ftell opt:
buf = `hello', size = 5
buf = `hello, world', size = 12
buf = `heyho, world', size = 12

As for other formats, the situation may be different from WAV and needs specific
analysis.

>
>
> Regards,
> Sven
>
>
>
>
> _______________________________________________
> SoX-devel mailing list
> SoX-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/sox-devel

[-- Attachment #1.2: Type: text/html, Size: 4789 bytes --]

[-- Attachment #2: test.c --]
[-- Type: application/octet-stream, Size: 533 bytes --]

#include <stdio.h>

int
main (void)
{
  char *bp;
  size_t size;
  FILE *stream;

  int end;

  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);

  end = ftell(stream);

  fseek (stream, 0, SEEK_SET);
  fprintf (stream, "heyho");

  fseek(stream, end, SEEK_SET);

  fclose (stream);
  printf ("buf = `%s', size = %ld\n", bp, size);

  return 0;
}


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]



[-- Attachment #4: Type: text/plain, Size: 158 bytes --]

_______________________________________________
SoX-devel mailing list
SoX-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sox-devel

  reply	other threads:[~2021-10-07  2:24 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-06 18:44 [PATCH] formats: disallow seeking in dynamic memory buffers Sven Neumann via SoX-devel
2021-10-07  2:24 ` Sun Zhenliang [this message]
2021-10-07  6:17   ` .Re: " Sven Neumann via SoX-devel
2021-10-07 12:22     ` Sun Zhenliang
2021-10-07 13:20       ` Sven Neumann via SoX-devel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://lists.sourceforge.net/lists/listinfo/sox-devel

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=OS3PR01MB5976D542EE0DAB4C146D0361C1B19@OS3PR01MB5976.jpnprd01.prod.outlook.com \
    --to=sox-devel@lists.sourceforge.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/sox.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).