How to read from buffer with feedback, so doesn't buffer overflow?

Question

Hi,

I have this code

#define BUFFER_LEN (2048)
static float buffer[BUFFER_LEN];
int readcount;

while ((readcount = sf_read_float(handle, buffer, BUFFER_LEN))) {
  // alsa play
}

which reads BUFFER_LEN floats from buffer, and returns the number of floats it actually read. "handle" tells sf_rad_float how big buffer is.

E.g. if buffer contains 5 floats, and BUFFER_LEN is 3, readcount would first return 3, and next time 2, and the while-loop would exit.

I would like to have a function that does the same.

Update

After a lot of coding, I think this is the solution.

#include <stdio.h>

int copy_buffer(double* src, int src_length, int* src_pos,
                float* dest, int dest_length) {

  int copy_length = 0;

  if (src_length - *src_pos > dest_length) {
    copy_length = dest_length;
    printf("copy_length1 %i\n", copy_length);
  } else {
    copy_length = src_length - *src_pos;
    printf("copy_length2 %i\n", copy_length);
  }

  for (int i = 0; i < copy_length; i++) {
    dest[i] = (float) src[*src_pos + i];
  }

  // remember where to continue next time the copy_buffer() is called
  *src_pos += copy_length;

  return copy_length;
}

int main() {

  double src[] = {1,2,3,4,5};
  int src_length = 5;

  float dest[] = {0,0};
  int dest_length = 2;

  int read;
  int src_pos = 0;
  read = copy_buffer(src, src_length, &src_pos, dest, dest_length);
  printf("read %i\n", read);
  printf("src_pos %i\n", src_pos);

  for (int i = 0; i < src_length; i++) {
    printf("src %f\n", src[i]);
  }

  for (int i = 0; i < dest_length; i++) {
    printf("dest %f\n", dest[i]);
  }

  return 0;

}

Next time copy_buffer() is called, dest contains 3,4. Running copy_buffer() again only copies the value "5". So I think it works now.

Although it is not very pretty, that I have int src_pos = 0; outside on copy_buffer().

It would be a lot better, if I instead could give copy_buffer() a unique handle instead of &src_pos, just like sndfile does.

Does anyone know how that could be done?

Answer 1

I started to look at it, but you could probably do it just as well: libsndfile is open source, so one could look at how sf_read_float() works and create a function that does the same thing from a buffer. http://www.mega-nerd.com/libsndfile/ has a download link.

Answer 2

If you would like to create unique handles, you can do so with malloc() and a struct:

typedef intptr_t HANDLE_TYPE;

HANDLE_TYPE init_buffer_traverse(double * src, size_t src_len);
int copy_buffer(HANDLE_TYPE h_traverse, double * dest, size_t dest_len);
void close_handle_buffer_traverse(HANDLE_TYPE h);

typedef struct 
{
    double * source;
    size_t source_length;
    size_t position;
} TRAVERSAL;

#define INVALID_HANDLE 0

/*
 * Returns a new traversal handle, or 0 (INVALID_HANDLE) on failure.
 *
 * Allocates memory to contain the traversal state.
 * Resets traversal state to beginning of source buffer.
 */
HANDLE_TYPE init_buffer_traverse(double *src, size_t src_len)
{
    TRAVERSAL * trav = malloc(sizeof(TRAVERSAL));

    if (NULL == trav)
        return INVALID_HANDLE;

    trav->source = src;
    trav->source_len = src_len;
    trav->position = 0;

    return (HANDLE_TYPE)trav;
}

/*
 * Returns the system resources (memory) associated with the traversal handle.
 */
void close_handle_buffer_traverse(HANDLE_TYPE h)
{
    TRAVERSAL * trav = NULL;

    if (INVALID_HANDLE != h)
        free((TRAVERSAL *)h);
}

int copy_buffer(HANDLE_TYPE h,
                float* dest, int dest_length)
{
    TRAVERSAL * trav = NULL;

    if (INVALID_HANDLE == h)
        return -1;

    trav = (TRAVERSAL *)h;

    int copy_length = trav->source_length - trav->position;
    if (dest_length < copy_length)
        copy_length = dest_length;

    for (int i = 0; i*emphasized text* < copy_length; i++)
        dest[i] = trav->source[trav->position + i];

    // remember where to continue next time the copy_buffer() is called
    trav->position += copy_length;

    return copy_length;
}

This sort of style is what some C coders used before C++ came into being. The style involves a data structure, which contains all the data elements of our 'class'. Most API for the class takes as its first argument, a pointer to one of these structs. This pointer is similar to the this pointer. In our example this parameter was named trav.

The exception for the API would be those methods which allocate the handle type; these are similar to constructors and have the handle type as a return value. In our case named init_buffer_traverse might as well have been called construct_traversal_handle.

There are many other methods than this method for implementing an "opaque handle" value. In fact, some coders would manipulate the bits (via an XOR, for example) in order to obscure the true nature of the handles. (This obscurity does not provide security where such is needed.)

In the example given, I'm not sure (didn't look at sndlib) whether it would make most sense for the destination buffer pointer and length to be held in the handle structure or not. If so, that would make it a "copy buffer" handle rather than a "traversal" handle and you would want to change all the terminology from this answer.

These handles are only valid for the lifetime of the current process, so they are not appropriate for handles which must survive restarts of the handle server. For that, use an ISAM database and the column ID as handle. The database approach is much slower than the in-memory/pointer approach but for persistent handles, you can't use in-memory values, anyway.

On the other hand, it sounds like you are implementing a library which will be running within a single process lifetime. In which case, the answer I've written should be usable, after modifying to your requirements.

Addendum

You asked for some clarification of the similarity with C++ that I mention above. To be specific, some equivalent (to the above C code) C++ code might be:

class TRAVERSAL
{
    double * source;
    size_t source_length;
    size_t position;

    public TRAVERSAL(double *src, size_t src_len)
    {
        source = src;
        source_length = src_len;
        position = 0;
    }

    public int copy_buffer(double * dest, size_t dest_len)
    {
        int copy_length = source_length - position;
        if (dest_length < copy_length)
            copy_length = dest_length;

        for (int i = 0; i < copy_length; i++)
            dest[i] = source[position + i];

        // remember where to continue next time the copy_buffer() is called
        position += copy_length;

        return copy_length;            
    }
}

There are some apparent differences. The C++ version is a little bit less verbose-seeming. Some of this is illusory; the equivalent of close_handle_buffer_traverse is now to delete the C++ object. Of course delete is not part of the class implementation of TRAVERSAL, delete comes with the language.

In the C++ version, there is no "opaque" handle.

The C version is more explicit and perhaps makes more apparent what operations are being performed by the hardware in response to the program execution.

The C version is more amenable to using the cast to HANDLE_TYPE in order to create an "opaque ID" rather than a pointer type. The C++ version could be "wrapped" in an API which accomplished the same thing while adding another layer. In the current example, users of this class will maintain a copy of a TRAVERSAL *, which is not quite "opaque."

In the function copy_buffer(), the C++ version need not mention the trav pointer because instead it implicitly dereferences the compiler-supplied this pointer.

sizeof(TRAVERSAL) should be the same for both the C and C++ examples -- with no vtable, also assuming run-time-type-identification for C++ is turned off, the C++ class contains only the same memory layout as the C struct in our first example.

It is less common to use the "opaque ID" style in C++, because the penalty for "transparency" is lowed in C++. The data members of class TRAVERSAL are private and so the TRAVERSAL * cannot be accidentally used to break our API contract with the API user.

Please note that both the opaque ID and the class pointer are vulnerable to abuse from a malicious API user -- either the opaque ID or class pointer could be cast directly to, e.g., double **, allowing the holder of the ID to change the source member directly via memory. Of course, you must trust the API caller already, because in this case the API calling code is in the same address space. In an example of a network file server, there could be security implications if "opaque ID" based on a memory address is exposed to the outside.

I would not normally make the digression into trustedness of the API user, but I want to clarify that the C++ keyword private has no "enforcement powers," it only specifies an agreement between programmers, which the compiler respects also unless told otherwise by the human.

Finally, the C++ class pointer can be converted to an opaque ID as follows:

typedef intptr_t HANDLE_TYPE;

HANDLE_TYPE init_buffer_traverse(double *src, size_t src_len)
{
    return (HANDLE_TYPE)(new TRAVERSAL(src, src_len));
}

int copy_buffer(HANDLE_TYPE h_traverse, double * dest, size_t dest_len)
{
    return ((TRAVERSAL *)h_traverse)->copy_buffer(dest, dest_len);
}

void close_handle_buffer_traverse(HANDLE_TYPE h)
{
    delete ((TRAVERSAL *)h);
}

And now our brevity of "equivalent" C++ may be further questioned.

What I wrote about the old style of C programming which relates to C++ was not meant to say that C++ is better for this task. I only mean that encapsulation of data and hiding of implementation details could be done in C via a style that is almost isomorphic to a C++ style. This can be good to know if you find yourself programming in C but unfortunately having learned C++ first.

PS

I just noticed that our implementation to date had used:

dest[i] = (float)source[position + i];

when copying the bytes. Because both dest and source are double * (that is, they both point to double values), there is no need for a cast here. Also, casting from double to float may lose digits of precision in the floating-point representation. So this is best removed and restated as:

dest[i] = source[position + i];