yuzu-emu/mbedtls
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity

Don't invalidate MPS reader buffers upon commit call

Browse source 
Previously, the semantics of mbedtls_mps_reader_commit() was to invalidate
all buffers previously fetched via mbedtls_mps_reader_get(), forbidding
any further use by the 'consumer'. This was in fact a necessary constraint
for the current implementation, which did some memory moving in
mbedtls_mps_reader_commit().

This commit simplifies the reader's semantics and implementation in
the following way:

- API: A call to mbedtls_mps_reader_commit() does no longer invalidate
       the buffers previously obtained via mbedtls_mps_reader_get().
       Instead, they can continue to be used until
       mbedtls_mps_reader_reclaim() is called.

       Calling mbedtls_mps_reader_commit() now only sets a marker
       indicating which parts of the data received through
       mbedtls_mps_reader_get() need not be backed up once
       mbedtls_mps_reader_reclaim() is called. Allowing the user
       to call mbedtls_mbedtls_reader_commit() multiple times
       before mbedtls_mps_reader_reclaim() is mere convenience:
       We'd get exactly the same functionality if instead of
       mbedtls_mps_reader_commit(), there was an additional argument
       to mbedtls_mps_reader_reclaim() indicating how much data
       to retain. However, the present design is more convenient
       for the user and doesn't appear to introduce any unnecessary
       complexity (anymore), so we stick with it for now.

- Implementation: mbedtls_mps_reader_commit() is now a 1-liner,
                  setting the 'commit-marker', but doing nothing else.

                  Instead, the complexity of mbedtls_mp_reader_reclaim()
                  slightly increases because it has to deal with creating
                  backups from both the accumulator and the current
                  fragment. In the previous implementation, which shifted
                  the accumulator content with every call to
                  mbedtls_mps_reader_commit(), only the backup from the
                  fragment was necessary; with the new implementation
                  which doesn't shift anything in
                  mbedtls_mps_reader_commit(), we need to do the
                  accumulator shift in mbedtls_mps_reader_reclaim().

Signed-off-by: Hanno Becker <hanno.becker@arm.com>
This commit is contained in:
Hanno Becker 2021-02-08 06:54:30 +00:00
parent 014f683ca9
commit 4f84e20eb0
2 changed files with 118 additions and 144 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

165
library/mps_reader.c
View file

@ -350,53 +350,14 @@ int mbedtls_mps_reader_get( mbedtls_mps_reader *rd,
int mbedtls_mps_reader_commit( mbedtls_mps_reader *rd )
{
unsigned char *acc;
mbedtls_mps_size_t aa, end, fo, shift;
mbedtls_mps_size_t end;
MBEDTLS_MPS_TRACE_INIT( "reader_commit" );
MBEDTLS_MPS_STATE_VALIDATE_RAW( rd->frag != NULL,
"mbedtls_mps_reader_commit() requires reader to be in consuming mode" );
acc = rd->acc;
end = rd->end;
if( acc == NULL )
{
MBEDTLS_MPS_TRACE( mbedtls_mps_trace_comment,
"No accumulator, just shift end" );
rd->commit = end;
MBEDTLS_MPS_TRACE_RETURN( 0 );
}
fo = rd->acc_share.frag_offset;
if( end >= fo )
{
MBEDTLS_MPS_TRACE( mbedtls_mps_trace_comment,
"Started to serve fragment, get rid of accumulator" );
shift = fo;
aa = 0;
}
else
{
MBEDTLS_MPS_TRACE( mbedtls_mps_trace_comment,
"Still serving from accumulator" );
aa = rd->acc_avail;
shift = end;
memmove( acc, acc + shift, aa - shift );
aa -= shift;
}
end -= shift;
fo -= shift;
rd->acc_share.frag_offset = fo;
rd->acc_avail = aa;
rd->commit = end;
rd->end = end;
MBEDTLS_MPS_TRACE( mbedtls_mps_trace_comment,
"Final state: (end=commit,fo,avail) = (%u,%u,%u)",
(unsigned) end, (unsigned) fo, (unsigned) aa );
MBEDTLS_MPS_TRACE_RETURN( 0 );
}
@ -406,7 +367,7 @@ int mbedtls_mps_reader_reclaim( mbedtls_mps_reader *rd,
unsigned char *frag, *acc;
mbedtls_mps_size_t pending, commit;
mbedtls_mps_size_t al, fo, fl;
MBEDTLS_MPS_TRACE_INIT( "reader_reclaim" );
MBEDTLS_MPS_TRACE_INIT( "mbedtls_mps_reader_reclaim" );
if( paused != NULL )
*paused = 0;
@ -429,6 +390,7 @@ int mbedtls_mps_reader_reclaim( mbedtls_mps_reader *rd,
{
MBEDTLS_MPS_TRACE( mbedtls_mps_trace_comment,
"No unsatisfied read-request has been logged." );
/* Check if there's data left to be consumed. */
if( commit < fo || commit - fo < fl )
{
@ -437,16 +399,28 @@ int mbedtls_mps_reader_reclaim( mbedtls_mps_reader *rd,
rd->end = commit;
MBEDTLS_MPS_TRACE_RETURN( MBEDTLS_ERR_MPS_READER_DATA_LEFT );
}
rd->acc_avail = 0;
rd->acc_share.acc_remaining = 0;
MBEDTLS_MPS_TRACE( mbedtls_mps_trace_comment,
"The fragment has been completely processed and committed." );
"Fragment has been fully processed and committed." );
}
else
{
int overflow;
mbedtls_mps_size_t acc_backup_offset;
mbedtls_mps_size_t acc_backup_len;
mbedtls_mps_size_t frag_backup_offset;
mbedtls_mps_size_t frag_backup_len;
mbedtls_mps_size_t backup_len;
mbedtls_mps_size_t acc_len_needed;
MBEDTLS_MPS_TRACE( mbedtls_mps_trace_comment,
"There has been an unsatisfied read-request with %u bytes overhead.",
(unsigned) pending );
"There has been an unsatisfied read with %u bytes overhead.",
(unsigned) pending );
if( acc == NULL )
{
@ -462,69 +436,61 @@ int mbedtls_mps_reader_reclaim( mbedtls_mps_reader *rd,
if( commit < fo )
{
/* No, accumulator is still being processed. */
int overflow;
MBEDTLS_MPS_TRACE( mbedtls_mps_trace_comment,
"Still processing data from the accumulator" );
overflow =
( fo + fl < fo ) || ( fo + fl + pending < fo + fl );
if( overflow || al < fo + fl + pending )
{
rd->end = commit;
rd->pending = 0;
MBEDTLS_MPS_TRACE( mbedtls_mps_trace_error,
"The accumulator is too small to handle the backup." );
MBEDTLS_MPS_TRACE( mbedtls_mps_trace_error,
"* Remaining size: %u", (unsigned) al );
MBEDTLS_MPS_TRACE( mbedtls_mps_trace_error,
"* Needed: %u (%u + %u + %u)",
(unsigned) ( fo + fl + pending ),
(unsigned) fo, (unsigned) fl, (unsigned) pending );
MBEDTLS_MPS_TRACE_RETURN(
MBEDTLS_ERR_MPS_READER_ACCUMULATOR_TOO_SMALL );
}
frag_backup_offset = 0;
frag_backup_len = fl;
acc_backup_offset = commit;
acc_backup_len = fo - commit;
}
else
{
/* Yes, the accumulator is already processed. */
int overflow;
MBEDTLS_MPS_TRACE( mbedtls_mps_trace_comment,
"The accumulator has already been processed" );
frag_backup_offset = commit;
frag_backup_len = fl - commit;
overflow = ( frag_backup_len + pending < pending );
if( overflow ||
al - fo < frag_backup_len + pending )
{
rd->end = commit;
rd->pending = 0;
MBEDTLS_MPS_TRACE( mbedtls_mps_trace_error,
"The accumulator is too small to handle the backup." );
MBEDTLS_MPS_TRACE( mbedtls_mps_trace_error,
"* Remaining size: %u", (unsigned) ( al - fo ) );
MBEDTLS_MPS_TRACE( mbedtls_mps_trace_error,
"* Needed: %u (%u + %u)",
(unsigned) ( frag_backup_len + pending ),
(unsigned) frag_backup_len, (unsigned) pending );
MBEDTLS_MPS_TRACE_RETURN(
MBEDTLS_ERR_MPS_READER_ACCUMULATOR_TOO_SMALL );
}
frag_backup_offset = commit - fo;
frag_backup_len = fl - frag_backup_offset;
acc_backup_offset = 0;
acc_backup_len = 0;
}
frag += frag_backup_offset;
acc += fo;
memcpy( acc, frag, frag_backup_len );
backup_len = acc_backup_len + frag_backup_len;
acc_len_needed = backup_len + pending;
overflow = 0;
overflow |= ( backup_len < acc_backup_len );
overflow |= ( acc_len_needed < backup_len );
if( overflow || al < acc_len_needed )
{
/* Except for the different return code, we behave as if
* there hadn't been a call to mbedtls_mps_reader_get()
* since the last commit. */
rd->end = commit;
rd->pending = 0;
MBEDTLS_MPS_TRACE( mbedtls_mps_trace_error,
"The accumulator is too small to handle the backup." );
MBEDTLS_MPS_TRACE( mbedtls_mps_trace_error,
"* Size: %u", (unsigned) al );
MBEDTLS_MPS_TRACE( mbedtls_mps_trace_error,
"* Needed: %u (%u + %u)",
(unsigned) acc_len_needed,
(unsigned) backup_len, (unsigned) pending );
MBEDTLS_MPS_TRACE_RETURN(
MBEDTLS_ERR_MPS_READER_ACCUMULATOR_TOO_SMALL );
}
MBEDTLS_MPS_TRACE( mbedtls_mps_trace_comment,
"Backup %u bytes into accumulator",
(unsigned) frag_backup_len );
"Fragment backup: %u", (unsigned) frag_backup_len );
MBEDTLS_MPS_TRACE( mbedtls_mps_trace_comment,
"Accumulator backup: %u", (unsigned) acc_backup_len );
rd->acc_avail = fo + frag_backup_len;
/* Move uncommitted parts from the accumulator to the front
* of the accumulator. */
memmove( acc, acc + acc_backup_offset, acc_backup_len );
/* Copy uncmmitted parts of the current fragment to the
* accumulator. */
memcpy( acc + acc_backup_len,
frag + frag_backup_offset, frag_backup_len );
rd->acc_avail = backup_len;
rd->acc_share.acc_remaining = pending;
if( paused != NULL )
@ -534,14 +500,13 @@ int mbedtls_mps_reader_reclaim( mbedtls_mps_reader *rd,
rd->frag = NULL;
rd->frag_len = 0;
rd->commit = 0;
rd->end = 0;
rd->pending = 0;
rd->commit = 0;
rd->end = 0;
rd->pending = 0;
MBEDTLS_MPS_TRACE( mbedtls_mps_trace_comment,
"Final state: aa %u, al %u, ar %u",
(unsigned) rd->acc_avail, (unsigned) rd->acc_len,
(unsigned) rd->acc_share.acc_remaining );
MBEDTLS_MPS_TRACE_RETURN( 0 );
}

97
library/mps_reader.h
View file

@ -31,7 +31,7 @@
* a 'consumer' which fetches and processes it in chunks of
* again arbitrary, and potentially different, size.
*
* Readers can be seen as datagram-to-stream converters,
* Readers can thus be seen as datagram-to-stream converters,
* and they abstract away the following two tasks from the user:
* 1. The pointer arithmetic of stepping through a producer-
* provided chunk in smaller chunks.
@ -54,36 +54,39 @@
* be satisfiable.
* - Repeat the above.
*
* From the perspective of the consumer, the state of the
* reader is a potentially empty list of input buffers that
* the reader has provided to the consumer.
* New buffers can be requested through calls to mbedtls_mps_reader_get(),
* while previously obtained input buffers can be marked processed
* through calls to mbedtls_mps_reader_consume(), emptying the list of
* input buffers and invalidating them from the consumer's perspective.
* The consumer need not be aware of the distinction between consumer
* and producer mode, because he only interfaces with the reader
* when the latter is in consuming mode.
* The abstract states of the reader from the producer's and
* consumer's perspective are as follows:
*
* From the perspective of the producer, the state of the reader
* is one of the following:
* - Attached: An incoming data buffer is currently
* being managed by the reader, and
* - Unset: No incoming data buffer is currently
* managed by the reader, and all previously
* handed incoming data buffers have been
* fully processed.
* - Accumulating: No incoming data buffer is currently
* managed by the reader, but some data
* from the previous incoming data buffer
* hasn't been processed yet and is internally
* held back.
* The Unset and Accumulating states belong to producing mode,
* while the Attached state belongs to consuming mode.
* - From the perspective of the consumer, the state of the
* reader consists of the following:
* - A byte stream representing (concatenation of) the data
* received through calls to mbedtls_mps_reader_get(),
* - A marker within that byte stream indicating which data
* need not be retained when the reader is passed back to
* the producer via mbedtls_mps_reader_reclaim().
* The marker can be set via mbedtls_mps_reader_commit()
* which places it at the end of the current byte stream.
* The consumer need not be aware of the distinction between consumer
* and producer mode, because he only interfaces with the reader
* when the latter is in consuming mode.
*
* Transitioning from Unset or Accumulating to Attached is
* done via calls to mbedtls_mps_reader_feed(), while transitioning
* from Consuming to either Unset or Accumulating (depending
* - From the perspective of the producer, the reader's state is one of:
* - Attached: The reader is in consuming mode.
* - Unset: No incoming data buffer is currently managed by the reader,
* and all previously handed incoming data buffers have been
* fully processed. More data needs to be fed into the reader
* via mbedtls_mps_reader_feed().
*
* - Accumulating: No incoming data buffer is currently managed by the
* reader, but some data from the previous incoming data
* buffer hasn't been processed yet and is internally
* held back.
* The Attached state belongs to consuming mode, while the Unset and
* Accumulating states belong to producing mode.
*
* Transitioning from the Unset or Accumulating state to Attached is
* done via successful calls to mbedtls_mps_reader_feed(), while
* transitioning from Consuming to either Unset or Accumulating (depending
* on what has been processed) is done via mbedtls_mps_reader_reclaim().
*
* The following diagram depicts the producer-state progression:
@ -94,9 +97,9 @@
* | | | |
* | | | |
* | feed +---------+---+--+ |
* +--------------------------------------> Attached <---+
* | / |
* +--------------------------------------> Consuming <---+
* +--------------------------------------> <---+
* | Attached |
* +--------------------------------------> <---+
* | feed, enough data available +---------+---+--+ |
* | to serve previous consumer request | | |
* | | | |
@ -108,6 +111,10 @@
* +--------+
* feed, need more data to serve
* previous consumer request
* |
* |
* producing mode | consuming mode
* |
*
*/
@ -141,7 +148,7 @@ struct mbedtls_mps_reader
/*!< The offset of the last commit, relative
* to the first byte in the accumulator.
* This is only used when the reader is in
* consuming mode, i.e. frag != NULL;
* consuming mode, i.e. \c frag != \c NULL;
* otherwise, its value is \c 0. */
mbedtls_mps_stored_size_t end;
/*!< The offset of the end of the last chunk
@ -306,8 +313,7 @@ int mbedtls_mps_reader_reclaim( mbedtls_mps_reader *reader,
* address of a buffer of size \c *buflen
* (if \c buflen != \c NULL) or \c desired
* (if \c buflen == \c NULL). The user hass ownership
* of the buffer until the next call to mbedtls_mps_reader_commit().
* or mbedtls_mps_reader_reclaim().
* of the buffer until the next call mbedtls_mps_reader_reclaim().
* \return #MBEDTLS_ERR_MPS_READER_OUT_OF_DATA if there is not enough
* data available to serve the read request. In this case,
* the reader remains intact, and additional data can be
@ -329,19 +335,22 @@ int mbedtls_mps_reader_get( mbedtls_mps_reader *reader,
mbedtls_mps_size_t *buflen );
/**
* \brief Signal that all input buffers previously obtained
* from mbedtls_writer_get() are fully processed.
* \brief Mark data obtained from mbedtls_writer_get() as processed.
*
* This function marks the previously fetched data as fully
* processed and invalidates their respective buffers.
* This call indicates that all data received from prior calls to
* mbedtls_mps_reader_fetch() has been or will have been
* processed when mbedtls_mps_reader_reclaim() is called,
* and thus need not be backed up.
*
* \param reader The reader context to use.
* This function has no user observable effect until
* mbedtls_mps_reader_reclaim() is called. In particular,
* buffers received from mbedtls_mps_reader_fetch() remain
* valid until mbedtls_mps_reader_reclaim() is called.
*
* \return \c 0 on success.
* \return A negative \c MBEDTLS_ERR_READER_XXX error code on failure.
* \param reader The reader context to use.
*
* \warning Once this function is called, you must not use the
* pointers corresponding to the committed data anymore.
* \return \c 0 on success.
* \return A negative \c MBEDTLS_ERR_READER_XXX error code on failure.
*
*/
int mbedtls_mps_reader_commit( mbedtls_mps_reader *reader );