No clue if our implementation would suffer the same fate
but the C standard states:
Where an argument declared as size_t n specifies
the length of the array for a function, n can have
the value zero […] pointer arguments on such a call
shall still have valid values, as described in 7.1.4.
Change-Id: Iee0dd9c948c6ad4b0d96309053127ab11111f04c
replace applicable calls to strlcpy with calls to strmemccpy
which null terminates on truncation
in theory the strmemccpy calls should be slightly faster since they
don't traverse the rest of the source string on truncation
but I seriously doubt there is too much of that going on in the code base
Change-Id: Ia0251514e36a6242bbf3f03c5e0df123aba60ed2
These operations can only be used in limited circumstances and have
exactly one user. bufgettail especially seems of dubious value; how
often do you need to read N bytes from the end of a file without
changing the file position?
strip_tags() was the only function using them, to strip off ID3v1
and APE tags off the end of buffered tracks. This would save only
32-192 bytes per track -- if the container format uses APE/ID3v1.
It hardly seems worth the effort.
Change-Id: I8fc3c1408517eda6126e75e76d76daea904b50eb
In some circumstances it was possible for a bitmap to overflow its
buffer and overwrite the next handle. The easiest way to trigger it
is with a highly compressed JPEG that is decoded to a large bitmap.
Because the JPEG file size is used to determine how much to allocate
this would cause an obvious buffer overflow when the JPEG is smaller
than the decoded bitmap. Fix this by using the decoded bitmap size as
the allocation size. Some overhead must be added to deal with JPEGs,
but it will be freed once the image is loaded.
A less obvious possibility is the fact that add_handle() will allow
a handle to be added even if there's not enough space for the entire
allocation. This is generally beneficial because it allows the first
part of a file to be loaded while waiting for space to free up, but
for bitmaps it is not valid because the whole image is loaded at once.
Hence if there is not actually enough space in the buffer, the bitmap
load can again overflow the actual free space and overwrite the next
handle.
The buffering code supports an H_ALLOCALL flag for allocations that
need the free space available immediately, so use it for bitmaps to
avoid that bug.
load_image() had a sketchy-looking check for free space which stopped
me from triggering the bug with simple tests, but since guessing the
free space is obviously a bad idea when the caller *knows* how much
free space there really is, remove that guess and let the caller tell
load_image() the real deal.
Change-Id: If62a58759705d83c16ee5b50f26bcbccc3f6c01f
Not sure what the comment is talking about - signed overflow
is undefined behavior and we don't use -fwrapv or other flags
to make it defined. I can't see how a compiler could abuse it
here, but the overflow is nonetheless easily avoided.
Change-Id: Ibed6d7c0d841db2aa86b9d8ba4c6a0d08c413354
Internally, buffering tries to load the entire album art file into the
audio buffer, which will fail if the file is larger than the buffer.
Playback.c interprets a file failing to buffer to mean that the buffer
is full, so it waits for more space and tries again. This results in a
deadlock since the file will never fit.
Change bufopen to return a new error condition when an image file will
not fit on the buffer because it is too large: ERR_BITMAP_TOO_LARGE.
Note that we arbitrarily set "too large" to be within 64KB of the
entire buffer size or larger, this could be adjusted if needed.
Change audio_load_albumart to pass through error messages from bufopen.
In playback.c, check to see why audio_load_albumart fails. If it fails
because the file is too large to buffer, simply ignore the file. If it
fails because the file would fit but the buffer is full, try again
later.
Change-Id: I66799ae26f124b495e1522fce7285332f4cf986f
Playback checked the files' presence before attempting to buffer
the track. Just get rid of that and save an extra open/close call.
It will find out if the path is bad when the metadata fails.
Fix some size_t/off_t conflation. No need to update plugin version
because no plugin actually uses bufopen().
Change-Id: I3db112449dc0b2eeb91c546f308880ac82494fc7
Paths are stored after the structure at their actual length plus
any aligment padding. In principle, any type of auxilliary data
could go there.
Change-Id: Ic5487dc4089781b5cc52414d1691ba6d9dc1893c
The location of the handle cannot be kept across calls to
shrink_handle() since it may move the structure. The error was
there in one place at the inception, corrected, then reintroduced.
Make shrink_handle() return the new location and use it, which
makes the side effects of the function clearer.
Change-Id: Icae6a0ad6f7bb0d6645b044cccfa4aef88db42ad
There's no need for it any longer since the list is now doubly-
linked. As a bonus, stack limits pose no barrier to the length of
the list.
Change-Id: I41c567f946b640ef1e3c2d93da2f5aef9a763c66
Does away the statically-allocated track list which frees quite
a fair amount of in-RAM size.
There's no compile-time hard track limit.
Recommended TODO (but not right away): Have data small enough use
the handle structure as its buffer data area. Almost the entire
handle structure is unused for simple allocations without any
associated filesystem path.
Change-Id: I74a4561e5a837e049811ac421722ec00dadc0d50
It sort of implemented an MRU cache but just kept track of the most
recent access and checked the one after it, otherwise searching from
the beginning.
Implement a true MRU list of all open handles. Handles of the current
track will tend to stay up front for faster access.
Switch to common linked list functions
Use double-linked lists to have insert, remove and move_handle
operations in O(1)-- no more searching to find the previous handle,
which is very often required.
Smaller code too. :)
Change-Id: I9ae8f3f96d225a5d54b94133f499268007274784
I'm not sure all the situations it affects, to be honest. The fix
aimed to address the strange symptom here:
http://forums.rockbox.org/index.php/topic,50793.0.html
It turns out that ringbuf_add_cross was used when handles were
butted up against one another with the first parameter equal to
the last, which it interprets as being an empty case when it should
be interpreted as full in the context it was used. To fix this,
introduce full/empty variants of ringbuf_add_cross and ringbuf_sub
and use them at the appropriate time.
The other way to address the problem is ensure there's always at
least a space byte between the end of one handle and the start of
another but this make the code a bit trickier to reason about than
using additional function variants.
bufopen() may yield after creating a handle and so do some more
locking so that the buffering thread doesn't mess things up by
moving anything or not seeing the yet-to-be linked-in allocation.
Add alignof() macro to use proper method to get alignment of
struct memory_handle. That should be useful in general anyway.
It's merely defined as __alignof__ but looks nicer.
Change-Id: If21739eaa33a4f6c084a28ee5b3c8fceecfd87ce
HAVE_IO_PRIORITY was defined for native targets with dircache.
It is already effectively disabled for the most part since dircache no
longer lowers its thread's I/O priority. It existed primarily for the
aforementioned configuration.
Change-Id: Ia04935305397ba14df34647c8ea29c2acaea92aa
Originating from 3661581
Some build clients finding their "standard" string.h's that don't declare
strlcpy?
Change-Id: I50d19c7cecf5ae96ee1855f77d3c2e1f42620108
It is trivial to obtain all required information from the allocated
handles without maintaining global indexes. In fact, it is less
complicated and increases general thread safety.
Other miscellaneous changes (some are nice to do at this time due to
required alterations, with some particularly more relevant than others):
* Handle value 0 will no longer be returned as a valid handle but all
failures will still return a negative value. Creates consistency with
buflib and removes the need to explicitly initialize them.
* Linking a new handle is delayed until explicitly
added by the code that called add_handle, keeping it invisible
until every operation succeeds, which is safer thread-wise. If anything
fails, the handle itself may just be abandoned rather than reqiring it
be freed.
* Dump the special handling to slow buffering when the PCM buffer
is low that calls PCM buffer functions. It doesn't seem to help much
of anything these days and it's a bit of a nasty hack to directly
tie those bits together. It can of course be put back (again!) if
there really is a need for it.
* Make data waiters ping the buffering thread more than just once if
the request is taking too long. Somehow I figured out how the requests
could get forgotten about but can't remember why months later after
making the change in my branch. :-)
* Neaten up some code by using (inline) functions and packing down
parameters; remember handle allocation and movement attributes in the
handle itself rather than figuring it out each time they're needed.
Change-Id: Ibf863370da3dd805132fc135e0ad104953365183
Reviewed-on: http://gerrit.rockbox.org/764
Reviewed-by: Michael Sevakis <jethead71@rockbox.org>
Tested: Michael Sevakis <jethead71@rockbox.org>
...by default where they would be interpreted as valid but not actually
be which would cause calls to buffering while it was not initialized.
Add BUFFER_EVENT_BUFFER_RESET to inform users of buffering that the
buffer is being reinitialized. Basically, this wraps all the
functionality being provided by three events (...START_PLAYBACK,
RECORDING_EVENT_START, RECORDING_EVENT_STOP) into one for radioart.c,
the only user of those events (perhaps remove them?) and closes some
loopholes.
Change-Id: I99ec46b9b5fb4e36605db5944c60ed986163db3a
It's not useful to do it since you need to write back the code to disk to be able to load it from memory, it also requires writing to an executable directory.
Keep it for the simulator for the sake of simulating.
git-svn-id: svn://svn.rockbox.org/rockbox/trunk@29261 a1c6a512-1295-4272-9138-f99709370657
- Support is limited to non-desync jpeg in id3v2 tags. Other formats (hopefully) follow in the future.
- Embedded album art takes precedence over files in album art files.
- No additional buffers are used, the jpeg is read directly from the audio file.
Flyspray: FS#11216
Author: Yoshihisa Uchida and I
git-svn-id: svn://svn.rockbox.org/rockbox/trunk@29259 a1c6a512-1295-4272-9138-f99709370657
