Martin Guy authored 6 months ago and Timothy B. Terriberry committed 6 months ago

On AIX, compilation fails saying

    src/http.c: In function 'op_http_conn_start_tls':
    src/http.c:1944:5: warning: ISO C forbids nested functions [-Wpedantic]
     1944 |     int                ip_len;
	  |     ^~~
    In file included from /usr/include/netinet/tcp.h:115,
		     from src/http.c:345:
    src/http.c:1944:24: error: expected '=', ',', ';', 'asm' or '__attribute__' before '.' token
     1944 |     int                ip_len;
    src/http.c:1944:24: error: expected expression before '.' token
    src/http.c:1949:5: error: 'ip_ff' undeclared (first use in this function); did you mean 'ip_fv'?
     1949 |     ip_len=0;

because `/usr/include/netinet/ip-h` contains

    #define  ip_len  ip_ff.ip_flen

The obvious solution os to rename the int variable to something else.

Instead of segfault, report OP_EFAULT if ogg_sync_buffer returns
a null pointer. This allows more graceful recovery by the caller
in the unlikely event of a fallible ogg_malloc call.

We do check the return value elsewhere in the code, so the new
checks make the code more consistent.

Thanks to https://github.com/xiph/opusfile/issues/36

 for reporting.

Signed-off-by: Timothy B. Terriberry <tterribe@xiph.org>
Signed-off-by: Mark Harris <mark.hsj@gmail.com>

When we were checking the current file offset to see if we should
 use it as the starting bisection point, we assumed that offset was
 larger than the start of the data range for that link (and
 consequently, inside the bisection range).
If there is a random page earlier in the file that happens to use
 the same serial number as a link we identified later in the file
 at file open time, and we had stopped reading there before the
 seek, then this assumption might not be true.

Ironically, it was not the case that contained the assertion that
 had trouble with such an offset.
It would fail the check that we were cutting off more than half the
 range, since we were actually cutting off a negative amount, and
 fall back to the midpoint of the link as the first bisection
 point.
However, the case below that (where the target comes after the
 current timestamp), we might have erroneously cut off the entire
 range (setting end to offset, which was less than begin), causing
 the seek to immediately fail.

Instead, validate the curent offset against both ends of the link
 before attempting to use it as the initial bisection point.
Thanks to Felicia Lim for the report.

Fixes #2331

Although link enumeration ensures the return value is in range, the
 order of operations allows the intermediate value pcm_total+diff
 to overflow the range of a 64-bit int.
Add parentheses to ensure this does not happen.
Thanks to Felcia Lim for the report.

Fixes #2330

When a file is very, very short (i.e., only one packet) and uses
 end-trimming, the apparent granule position preceding the first
 sample in the first packet can underflow.
We were computing this value by subtracting the packet duration
 from the computed per-packet granule position and expecting this
 computation to always succeed.
Because it could fail in the presence of end-trimming on the first
 packet (ironically, exactly the situation where the short-circuit
 is helpful), it would leave the value uninitialized, and then use
 it in a comparison, which is undefined behavior.
The correct solution is to check for failure and force the previous
 page's granule position to 0 in this case.

We were attempting to ensure a minimum spacing between granule
 positions when guessing the start of a link location.
However, we took a strictly-positive granule position, added a
 fixed increment with op_granpos_add(), and checked if
 op_granpos_add() failed.
op_granpos_add() only fails if the sum would have overflowed past
 zero, which can never happen when adding two strictly positive
 granule positions.
Instead, we need to check if the result becomes negative (which is
 a legal granule position, but violates our assumptions in the
 search).

Thanks to Felicia Lim for the report.

Prefer the secure protocol by default. We do not update schema
links since they're often standardized with protocol included.

Note that the link to the Windows certificate store patch
in the openssl.org bug tracker requires a login now.

Use a secure link by default. Drop the redundant www subdomain
to keep within the 70-column width of the header.

The actual guarantees we are making in op_read_native() are:
- if _pcm == NULL, then _buf_sz <= 0 (requirement on the caller),
- op_get_packet_duration() will succeed and return a positive value
   no larger than 120*48 (guaranteed by op_collect_audio_packets()
   filtering out any packets with invalid TOC sequences), and
- nchannels is a small number greater than 0 (guaranteed by the
   validation in opus_parse_head()).
However, trying to assert these things is not enough to convince
 clang to take the nsamples*nchannels>_buf_sz or
 duration*nchannels>_buf_sz branches when _pcm==NULL, so instead
 we have to be a bit more direct.

There really isn't anything that guarantees we can find timestamps
 within the bounds of the link, or within 2 billion samples of the
 target, if people resort to nasty things like swapping out all of
 the underlying file data on us at the right moment.
Reported by clang's static analysis in
 https://github.com/xiph/opusfile/issues/16

Thanks to kcgen for filing the report.

Reported by clang's static analysis in
 https://github.com/xiph/opusfile/issues/16
This would have taken about 22 TB of junk data before the first
 decoded sample to trigger, so not very likely to occur in
 practice.

Thanks to kcgen for filing the report.

 src/opusfile.c:3242:18: warning: implicit conversion from 'unsigned
       int' to 'float' changes value from 4294967295 to 4294967296
       [-Wimplicit-int-float-conversion]
           r=seed*OP_PRNG_GAIN;
                  ^~~~~~~~~~~~
 src/opusfile.c:3179:29: note: expanded from macro 'OP_PRNG_GAIN'
 # define OP_PRNG_GAIN (1.0F/0xFFFFFFFF)
                            ~^~~~~~~~~~

The ftime() function, introduced in V7 Unix (1979), gets the current
time in seconds and milliseconds, and time zone information.  It was
marked as a legacy interface in POSIX.1-2001, and removed altogether
from POSIX.1-2008.  The gettimeofday() function, originally from
4.1 BSD, gets the current time in seconds and microseconds, and optional
time zone information, and was marked as obsolete in POSIX.1-2008
although it was kept in the standard.  The POSIX recommended function
for getting time with sub-second resolution is clock_gettime(), which
was introduced in POSIX.1b-1993 and is now part of the base POSIX
standard; it supports multiple clocks and nanosecond resolution.
Additionally the function timespec_get() was introduced in C11 and also
supports nanosecond resolution.

To support dates beyond the year 2038, glibc and other libraries are
being updated to support 64-bit time_t even on 32-bit architectures,
requiring new implementations of interfaces that work with time.  As
part of this effort, the ftime() function was deprecated in glibc 2.31
(released February 1, 2020), a warning is now issued when building code
that uses this function, and removal is planned for a future version of
glibc (https://sourceware.org/pipermail/libc-announce/2020/000025.html).

ftime() is used in http.c to measure time intervals with millisecond
resolution.  To avoid the glibc 2.31 deprecation warning and further
issues when the function is removed entirely from glibc, clock_gettime()
is now used instead when it is available in the C library, as it is on
current Linux systems.  Prior to glibc 2.17, clock_gettime() required
linking with librt; on such systems ftime() will continue to be used, to
avoid an additional library dependency.  macOS provides clock_gettime()
starting in macOS 10.12; earlier versions will continue to use ftime().
Windows provides ftime() but not clock_gettime(), so ftime() will
continue to be used on Windows.

ftime(), gettimeofday(), and clock_gettime() with the CLOCK_REALTIME
clock get the "real time", which is subject to jumps if set by an
administrator or time service.  The CLOCK_MONOTONIC clock does not have
this problem and is more suitable for measuring time intervals.  On
Linux, the CLOCK_BOOTTIME clock measures the time since last boot and is
the same as CLOCK_MONOTONIC except that the Linux CLOCK_MONOTONIC clock
does not advance when the system is suspended.  Because it is used to
measure time intervals, CLOCK_BOOTTIME or CLOCK_MONOTONIC are used when
available, when clock_gettime() is used.  However the only clock
required by POSIX.1-2008 is CLOCK_REALTIME, so that will be used if the
other clocks are not available.

It is possible for us to buffer multiple out-of-sequence pages with no packets
 on them before getting one that does have packets.
In this case, libogg will report each hole in the page sequence numbers
 separately.
Since we were only checking for the actual packets once after encountering a
 hole, if the total number of holes was even, we could exit
 op_fetch_and_process_page() with valid packets still in the libogg buffer.
Then, if the next page had a lot of packets, we might wind up with a total of
 more than 255 of them, overflowing our stack buffer for their durations.
That's bad.

Instead, make sure we always drain all hole reports from libogg any time we
 encounter one, to ensure we get the actual packets behind it.

Thanks to Felicia Lim for the report.

The C standard says that calling library functions (including
 memcpy) with invalid arguments (including a NULL pointer) is
 undefined behavior unless otherwise noted (which memcpy doesn't).
op_filter_read_native() invokes op_read_native() with NULL for the
 _pcm buffer, which triggers such a memcpy invocation.
Even though it should be perfectly fine in practice to pass NULL to
 memcpy when copying zero bytes, don't do it.

Thanks to a person who did not wish to be credited for the report.

The seeking functions on Windows internally dispatch to
 SetFilePointer(), whose behavior is undefined if you do not call
 it on "a file stored on a seeking device".
Check the type of file when it is opened and manually force seeks
 to fail if we do not have such a file.

Thanks to L W Anhonen for the report and Mark Harris for pointing
 out the solution used by opus-tools to avoid this problem.

When seeking using SEEK_END, the win32-specific implementation of
 op_fseek() would add the offset to the file size to get the
 absolute seek position.
However, op_mem_seek() and op_http_stream_seek() would subtract the
 offset instead.
The documentation of fseek() is not at all clear which behavior is
 correct, but I believe that the op_fseek() behavior is.

This inconsistency didn't matter for opusfile in practice, because
 we only ever use SEEK_END with an offset of 0.
However, the user can also open files with our API and use the
 resulting callbacks for their own purposes, so it would be good to
 be consistent for them.

Thanks to a person who did not wish to be credited for the report.

Stefan Strogin authored 6 years ago and Ralph Giles committed 6 years ago

LibreSSL is not yet fully API compatible with OpenSSL 1.0.2 and later,
However many APIs from OpenSSL 1.0.2 and 1.1 are already implemented in
LibreSSL 2.7.0 and later. Old approach works in newer LibreSSL version
as well, but it's not nice to force deprecated functions on LibreSSL
users.

Add additional conditionals for new LibreSSL versions to use the
available new APIs.

Signed-off-by: Ralph Giles <giles@thaumas.net>

Stefan Strogin authored 6 years ago and Timothy B. Terriberry committed 6 years ago

LibreSSL defines OPENSSL_VERSION_NUMBER to 0x20000000L, but its API is
compatible with OpenSSL 1.0.1.
Therefore redefine OPENSSL_VERSION_NUMBER to 0x1000115fL (1.0.1u) if
LibreSSL is used.

Fixes: #2327

Mark Harris authored 6 years ago and Timothy B. Terriberry committed 6 years ago

The comment must be just before the label to suppress the warning.

Mark Harris authored 6 years ago and Timothy B. Terriberry committed 6 years ago

Silence warning: Value stored to 'comment_lengths' is never read

James Zern authored 7 years ago and Timothy B. Terriberry committed 7 years ago

Check for overflow with a negative diff.

Signed-off-by: Timothy B. Terriberry <tterribe@xiph.org>

James Zern authored 7 years ago and Timothy B. Terriberry committed 7 years ago

Calculate discard_count directly with op_granpos_diff. this works
because _pcm_offset == (target_gp - pcm_start) and diff == (gp -
pcm_start).

Signed-off-by: Timothy B. Terriberry <tterribe@xiph.org>

We very carefully ensured _cur_link + 1 was in bounds, and then
 dereferenced nlinks + 1 (guaranteed to be out of bounds) instead.
Introduced in commit f83675eb.

Thanks to the Google Autfuzz project for the report.

Fixes #2326

op_fopen() and op_freopen() declare these arguments as non-NULL,
 so when building with mingw, the compiler reasonably complains
 when we check to see if they're NULL.
We could remove the OP_ARG_NONNULL tags, but the behavior of
 _wopen/_wfreopen appears to be to crash on NULL for either
 parameter.
On Linux, the behavior appears to be to handle a NULL path (fopen
 returns NULL with errno set to EFAULT, and freopen returns the
 passed FILE * with errno set to EFAULT), but crash on a NULL mode.
Keeping the OP_ARG_NONNULL tags promises that passing NULL results
 in undefined behavior, which is at least consistent with the
 behavior being different on different platforms.
It's also consistent with the ABI promises of previous releases,
 which compilers linking against libopusfile might have taken
 advantage of.

We inherited the term "source" from vorbisfile's "datasource", but
 were using it interchangeably with stream.
At least one user did not even realize the that the _source
 parameter passed to op_open_callbacks() was the same as the
 _stream parameter taken by those callbacks, which is reasonable
 because we never said so.
Consistently use "stream" instead of "source" in both the
 documentation and the code.

These two asserts used to be in separate branches, but the code was
 consolidated into a single branch by e6350334.
However, the asserts were not updated at the same time.
With an IPv6 address, the sockaddr_in assert would always fail.

Thanks to Mark Harris for the report.

The tags have two possible representations of the binary suffix
 data.
An implicit one: if comment_lengths and user_comments are NULL,
 then the length of the binary suffix data is 0 and the pointer to
 that data is also implicitly taken to be NULL.
And an explicit one: if comment_lengths and user_comments are
 non-NULL, then comment_lengths[comments] and
 user_comments[comments] contains the length and pointer to the
 binary suffix data (where "comments" is the number of user
 comments).

The implicit one allows us to initialize a tags struct without
 doing any allocations, which ensures it always succeeds.
op_tags_ensure_capacity() may upgrade the implicit representation
 to the explicit representation.
However, when doing so, it stores the implicit (0, NULL) values
 in their new explicit locations at the end of the array assuming
 the requested capacity will become the new size.
If the caller of op_tags_ensure_capacity() then fails before
 enlarging the array to that size, then comment_lengths and
 user_comments will be non-NULL, but the explicit locations for the
 binary suffix data for the _old_ size may not have been
 initialized.

In particular, in opus_tags_parse_impl(), if there was exactly one
 comment, and any of the three checks at the start of the main loop
 failed, then the explicit locations for the binary suffix data
 would remain uninitialized, and the call to opus_tags_clear() in
 the caller would attempt to free them.
For counts larger than 1, the extra line marked "Needed by
 opus_tags_clear()" will update the explicit location as the array
 grows, but with a count of 1 this line never gets a chance to run.

This patch fixes the problem by making op_tags_ensure_capacity()
 promote the implicit representation to explicit at _both_ the old
 and new array sizes.
We could have fixed up opus_tags_parse_impl() to do this instead,
 as suggested in the original bug reports, but doing it this way
 ensures that the tags are always in a valid state when
 op_tags_ensure_capacity() returns.

Thanks to the Google AutoFuzz Team for the report.

Fixes #2324
Fixes #2325

op_raw_seek() will fail if you try to seek to a byte position
 beyond the end of the file.
However, the "end" is defined in terms of _of->end, which is
 specifically the end of the last Ogg page found in the underlying
 source (excluding any trailing non-Ogg data).
op_raw_total(_of,-1) returns the total size of the stream by using
 _of->end, but it was also subtracting the offset of the first Opus
 page in the first link.
Since there might have been other Ogg streams concurrently
 multiplexed with the first Opus stream whose BOS pages appear
 first, or there might simply be non-Ogg junk at the start, that
 left the caller with no way to determine the valid range of byte
 offsets that could be passed to op_raw_seek().

Instead, make op_raw_total() pretend the first link starts at
 offset 0, and explicitly document that it's what defines the range
 of valid values to op_raw_seek().
This is how our own seeking_example.c was using it, anyway.

Previously, when we encountered a hole (a gap in the page sequence
 numbers), we would save off all of the packets from the first page
 after the hole, but not timestamp them.
That meant when they were actually decoded, op_pcm_tell() would
 report a timestamp of 0 until reaching the last packet on that
 page.

Instead, handle holes just like a raw seek.
We reset the granule position tracking, and attempt to timestamp
 packets backwards from the end of the page.
If the first page after the hole is an EOS page, we just throw it
 away (rather than risk playing invalid samples due to incorrect
 end-trimming).
We also throw away the first 80 ms of audio after a hole, to allow
 the decoder state to reconverge.

This patch also updates the example to report the hole and
 continue decoding, rather than immediately stopping when a hole is
 encountered, in order to test the above features.

Just in case some bozo makes a chained stream with 272,389 links
 with 16 samples in each (coded at 16 Mbps, including overheads).
This avoids quadratic behavior for simple straight-through
 playback: we no longer do a linear search on each chain boundary
 or each call to op_pcm_tell().
N seeks with M chains still requires O(N*log(M)) work.

No code changes.

As of version 1.0.2, OpenSSL can finally do automatic hostname
 validation for us.
Their implementation is likely to have received much better review
 than ours, and there are other good reasons to prefer it, so use it
 when we can.