reading that from serial port in c - array indexing issue

Question

I have some c code that reads from a serial port into a buffer continiously and when it has read 100 bytes it writes them over a websocket using the function log_write(). Some of the data is missing so I think I'm getting undefined behaviour. Is there anything obviously wrong with the code?

Specifcally on the lines below should I be adding a 1 to rdlentotal? Is it overwriting the last char of the previous read?

Also should I be null terminating the buffer after the last char that is read? How would I do this?

rdlen = read(fd_joule, &ibuf[rdlentotal], sizeof(ibuf)); rdlentotal += rdlen; /*keep track of total bytes read*/

int rdlen=0, rdlentotal = 0;
char ibuf[1024];

memset(&ibuf[0], 0, sizeof(ibuf)); /*clear the buffer*/

while (1) {

    /*read from serial port*/
    rdlen = read(fd_joule, &ibuf[rdlentotal], sizeof(ibuf));
    rdlentotal += rdlen; /*keep track of total bytes read*/

    if (rdlentotal > 100) { /*when you have 200 bytes write the websocket*/
        log_write("%s", &ibuf); /*write to websocket*/
        memset(&ibuf[0], 0, sizeof(ibuf)); /*clear buffer*/
        rdlentotal = 0; /*rest byte counter */
        rdlen = 0;
    }

    if (rdlen < 0) {
        fprintf(stderr, "rdlen les than 0\r\n");
    }

}

Updated code suggestions from Chux:

static void *serial_logging_thread() {

    ssize_t rdlen = 0, rdlentotal = 0;
    char ibuf[1024 + 1];

    memset(&ibuf[0], 0, sizeof(ibuf)); /*clear the buffer*/

    while (1) {

        /*read from serial port write to log file*/
        rdlen = read(fd_joule, &ibuf[rdlentotal], sizeof(ibuf) - 1 - rdlentotal);
        rdlentotal += rdlen;

        if (rdlentotal > 200) { /*when you have 200 bytes write to file and websocket*/

            ibuf[rdlentotal + 1] = '\0'; /*null terminate*/
            log_write("%s", &ibuf); /*write to websocket*/

            memset(&ibuf[0], 0, sizeof(ibuf)); /*clear buffer*/
            rdlentotal = 0; /*rest byte counter */
            rdlen = 0;
        }

        if (rdlen < 0) {
            LOG("error reading serial port, rdlen less than 0\r\n");
        }
    }

}

Declaration for log_write().

int log_write(const char *fmt, ... /* arguments */){

read(fd_joule, &ibuf[rdlentotal], sizeof(ibuf)); --> read(fd_joule, &ibuf[rdlentotal], sizeof(ibuf) - rdlentotal); — chux - Reinstate Monica
Tell us please - can the stream of bytes have NULs in it? If so, you are already broken, as chux hints at below. — Martin James
samsung gather: Rather than add a copy of an answer to your question, 1) Review What should I do when someone answers my question? 2) roll back your question to the previous version. — chux - Reinstate Monica

chux - Reinstate Monica chux - Reinstate Monica · Accepted Answer · 2017-11-30T16:06:14

Is there anything obviously wrong with the code?

Yes, see below.

Specifically on the lines below should I be adding a 1 to rdlentotal?

No.

Is it overwriting the last char of the previous read?

No.

... should I be null terminating the buffer after the last char that is read?

Yes, if buf is treated as a string. Yet since input may have '\0' bytes in it, more robust code would treat buf as a character array and pass buf and its length used to the logging functions. Also see @Martin James

How would I do this (null terminate the buffer) ?

See #2 below.

Code has at least the following problems:

Buffer overrun possibility.

// rdlen = read(fd_joule, &ibuf[rdlentotal], sizeof(ibuf));
rdlen = read(fd_joule, &ibuf[rdlentotal], sizeof(ibuf) - rdlentotal);

Assuming log_write("%s", &ibuf); is similar to printf(), code is attempting to print ibuf, which may not be a string and it is not certainly null character terminated. Think of what happens if the first read was sizeof(ibuf) characters long. Instead, insure space and append a null character.

char ibuf[1024 + 1];  // add 1 here                   - 1 below
...
rdlen = read(fd_joule, &ibuf[rdlentotal], sizeof ibuf - 1 - rdlentotal);
...
ibuf[rdlentotal + rdlen] = '\0';  // Code is certain to have memory for the \0
...
log_write("%s", &ibuf);  // The use of & here is likely not needed.

read() returns a ssize_t, not int. Use that type and check for errors before rdlentotal += rdlen;

ssize_t rdlen = read(...
if (rdlen == -1) {
  LOG("error reading serial port, rdlen less than 0\r\n");
  continue;
}
rdlentotal += rdlen;

More robust to use size_t for array lengths than int
```
// int rdlentotal = 0;
size_t rdlentotal = 0;
```

Minor: Comment does not reflect code.

// if (rdlentotal > 100) { /*when you have 200 bytes write the websocket*/
if (rdlentotal > 100) { /*when you have more than 100 bytes write the websocket*/

reading that from serial port in c - array indexing issue

2 Answers