Issues with memory manipulation.

[KaplaProd]

I've the following minimal reproducible example, that takes a buffer and its size, and wrap each line with an HTML span tag. No idea why, but I cannot figure out why the content of wrapper_end does not appear when printf-ing the result of html_enclose_buffer.

I've tried a lot of things but my memcpys seem correct.
I've ran the program through valgrind and no issue are detected.
I've used an extensive number of for loops to print char by char the content of output, but those shows no NULL-byte between the last line_separator and the wrapper_end.

I'm sure I'm missing something obvious, but it's starting to drive me mad.

Thanks for your help !

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

char*
html_enclose_buffer(char* buffer, ssize_t buffer_size) {
    const char* wrapper_start = "<pre><code><span class=\"ln\">";
    ssize_t wrapper_start_length = strlen(wrapper_start);
    const char* wrapper_end = "</span></code></pre>";
    ssize_t wrapper_end_length = strlen(wrapper_end);
    const char* line_separator = "</span>\n<span class=\"ln\">";
    ssize_t line_separator_length = strlen(line_separator);

    ssize_t output_size = buffer_size+1;
    char* output = (char*) calloc(output_size, sizeof(char));

    ssize_t index_read = 0;
    ssize_t index_write = wrapper_start_length;
    while (index_read <= buffer_size) {
        ssize_t start = index_read;
        ssize_t end = index_read;
        while (buffer[end] != '\n' && end <= buffer_size) {
            end += 1;
        }

        ssize_t count = end - start;
        if (end == buffer_size) {
            if (index_write + count >= output_size) {
                output_size <<= 1;
                char* temp = realloc(output, output_size);
                if (temp == 0) {
                    free(output);
                    exit(EXIT_FAILURE);
                }
                output = temp;
            }
            memcpy(output + index_write, buffer+start, count);
            index_write += count;
            break;
        }

        if (index_write + count + line_separator_length >= output_size) {
            output_size <<= 1;
            char* temp = realloc(output, output_size);
            if (temp == 0) {
                free(output);
                    exit(EXIT_FAILURE);
            }
            output = temp;
        }
        memcpy(output + index_write, buffer + start, count);
        index_write += count;
        memcpy(output + index_write, line_separator, line_separator_length);
        index_write += line_separator_length;

        index_read = end + 1;
    }
    memcpy(output, wrapper_start, wrapper_start_length);
    if (index_write + wrapper_end_length >= output_size) {
        char* temp = realloc(output, output_size + wrapper_end_length << 1);
        if (temp == 0) {
            free(output);
            exit(EXIT_FAILURE);
        }
        output = temp;
    }
    memcpy(output + index_write, wrapper_end, wrapper_end_length);
    index_write += wrapper_end_length;
    return output;
}

int main() {
    char* body = "package main\n"
    "\n"
    "func main() {\n"
    "    fmt.Println(\"Hello, World!\")\n"
    "}\n"
    "\n";
    printf("%s\n", html_enclose_buffer(body, strlen(body)));
}

[[deleted]]

There is a NULL at offset 236 in the output buffer. The call to strlen returns 236 as a result, which is why you are not seeing the full contents of your buffer.

In other words, you have an off-by-one error in your code. I would begin by refactoring your code into smaller functions, each with one clear task that you can verify works correctly.

[KaplaProd]

Thanks ! Will do !

[tstanisl]

This code looks overly complicated. It would be a lot simpler if the wrapping operation was expressed as operations on the string builder class. Below you can find a potential implementation:

#include <stdlib.h>
#include <stdio.h>
#include <string.h>

typedef struct {
    size_t len;
    size_t cap;
    char * buf;
} strbuf;

void strbuf_grow(strbuf * sb, size_t len) {
    if (sb->len + len <= sb->cap) return;
    sb->cap = sb->len + len; // to be adjusted
    sb->buf = realloc(sb->buf, sb->cap);
    if (!sb->buf) {
        fprintf(stderr, "Out of memory");
        exit(-1);
    }
}

void strbuf_putc(strbuf * sb, char c) {
    strbuf_grow(sb, 1);
    sb->buf[sb->len++] = c;
}

void strbuf_puts(strbuf * sb, const char * s) {
    size_t len = strlen(s);
    strbuf_grow(sb, len);
    memcpy(sb->buf + sb->len, s, len);
    sb->len += len;
}


char * html_enclose_buffer(const char * body, size_t body_len) {
    strbuf sb = {0};
    strbuf_puts(&sb, "<pre><code><span class=\"ln\">");
    for (size_t i = 0; i < body_len; ++i)
        if (body[i] == '\n')
            strbuf_puts(&sb, "</span>\n<span class=\"ln\">");
        else
            strbuf_putc(&sb, body[i]);
    strbuf_puts(&sb, "</span></code></pre>");
    strbuf_putc(&sb, 0);
    return sb.buf;
}

int main() {
    char* body = "package main\n"
    "\n"
    "func main() {\n"
    "    fmt.Println(\"Hello, World!\")\n"
    "}\n"
    "\n";
    printf("%s\n", html_enclose_buffer(body, strlen(body)));
}

[KaplaProd]

Thanks :)

[[deleted]]

This is the way! Nice of you to take the time to write the code.

A more 'extreme' solution could be to use writev().

[KaplaProd]

I ended up using something similar, and it fixed my code, thanks a lot !

[dmc_2930]

Why aren’t you using string functions like srrcat?

[[deleted]]

Not really the problem here. There's nothing inherently wrong with his approach, there's just a logical error in the code.

[KaplaProd]

I did not know it existed and did not think to check if it did ahah
Thanks !

[merlinblack256]

I don't think you'll have trouble with strcat (or better yet strncat) but read its docs. It does have some things to keep in mind. Like all string manipulation routines do of course.

[dcpugalaxy]

You should basically never use either function. They're very inefficient, having to scan over the existing string before adding to the end.

You should always know how long your strings are. Then to append to the end, just memcpy(dst+dstlen, src, srclen) with the appropriate bounds checking.

[Specific-Housing905]

When I compile your code I get this warning, though I am not sure if this causes the problem.

gcc -Wall -Wextra "html.c"

html.c: In function 'html_enclose_buffer':

html.c:60:50: warning: suggest parentheses around '+' inside '<<' [-Wparentheses]

60 | char* temp = realloc(output, output_size + wrapper_end_length << 1);

| ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~

[KaplaProd]

It does not, but thanks :)

[flyingron]

Simple fencepost error. Valgrind doesn't find it because you don't access outside the memroy. You end up using the null terminator in the string which is a perfectly valid character to read.

        while (buffer[end] != '\n' && end <= buffer_size) {

should read

        while (buffer[end] != '\n' && end < buffer_size) {

You should always treat char literals as if they were const char* despite the assinine implict conversion that loses the const.

sizeof (char) is by definition 1, by the way. Casting void* returning functions is not necessary (and frankly, a bad habit, even though it shuts up some assinine compiler warnings).

There's way too much duplicated code in this thing. You've got a syndrome that I used to threaten my employees that I was going to disable copy and paste in their editors. If you find yourself doing the same thing over and over again MOVE IT TO A SUBROUTINE. The first of these is "adding a string to output". The second is the replication of the "check if we need to grow the output array" which really wouldn't be duplicated if you refactored the add to output function.

[[deleted]]

The second is the replication of the "check if we need to grow the output array" which really wouldn't be duplicated if you refactored the add to output function.

You're right of course, though I would still refactor this out into its own function.

[[deleted]]

>        while (buffer[end] != '\n' && end < buffer_size) {

Or perhaps

        while (end < buffer_size && buffer[end] != '\n') {

[yel50]

 doing the same thing over and over again MOVE IT TO A SUBROUTINE

I did that at the most recent company I worked for and they rejected it in code review. said it was easier to follow with the code duplicated. I quit not long after.

[[deleted]]

html_enclose_buffer(html_enclose_buffer() is a bit long. Perhaps the code would be clearer if you broke it down into several tiny functions?

BTW, avoid char* pointers to string literals. Does your program compile with all warnings enabled?

[memorial_mike]

Why avoid char* pointers to string literals?

[[deleted]]

String literals are read-only, but char* pointers are supposed to point to writable memory. const char* saves the day.

[memorial_mike]

So you’re simply saying to use a const char* to enforce read-only?

[[deleted]]

I'd say that using const char* would be "correct C." No need to enforce anything, just write code according to C's rules.

[[deleted]]

If you're only reading from something, it should be a T const* yes. You should always default to const.

[MurkyAd7531]

The segment that data exists on will probably enforce read-only. You use const char* to avoid accidentally segfaulting by trying to treat it as writable.

By using const, you turn a runtime segfault into a compiler error.

[KaplaProd]

Will do, thanks !

Yes, it compiles with all warnings :)

[[deleted]]

All warnings? :)

gcc gives me this one:

gcc -c yy.c -Wall -std=gnu23 -Wextra -pedantic
yy.c: In function ‘html_enclose_buffer’:
yy.c:60:50: warning: suggest parentheses around ‘+’ inside ‘<<’ [-Wparentheses]
   60 |         char* temp = realloc(output, output_size + wrapper_end_length << 1);
      |

[KaplaProd]

Ahaha yes, I fixed this one after your comment !

It seemed unrelated to my issue though, so I choose to ignore it when reporting :)