Out of bounds memory access with more than 1 pixel per byte pixel formats, e.g. SIXEL_PIXELFORMAT_G1

[xyzzy42]

Some pixel formats put more than one pixel in a single byte. For example, SIXEL_PIXELFORMAT_G1 encodes a pixel as a single bit and packs 8 pixels into 1 byte.

sixel_helper_compute_depth() returns 1 for these formats. I.e., 1 pixel per byte, not 8. So width * height * depth is too large for the image data when using these formats.

This is then used in, e.g., sixel_encoder_output_without_macro():

    depth = sixel_helper_compute_depth(pixelformat);
    size = (size_t)(width * height * depth);
    p = (unsigned char *)sixel_allocator_malloc(encoder->allocator, size);
    memcpy(p, pixbuf, (size_t)(width * height * depth));

Here the memcpy will read too much data from pixbuf. And also the allocated buffer is larger than it needs to be.

Common solution is to create a helper than returns number of bytes in one line, rather than in one pixel. The width must be passed as well. This avoids the need to return fractional value. Also can take into account padding line to end on a byte boundary.

int compute_line_length(int pixelformat, int width) {
  if (pixelformat_less_than_1_byte) {
    //  Just example for < 1 byte formats
    switch (pixelformat) {
      case SIXEL_PIXELFORMAT_G1: bpp = 1; break;
      case SIXEL_PIXELFORMAT_G2: bpp = 2; break;
       …
    }
    return (width * bpp + 7) / 8;
  } else {
    return sixel_helper_compute_depth(pixelformat) * width;
  }
}

// usage
  size = (size_t)height * compute_line_length(pixelformat, width);

P.S., (size_t)(width * height * depth) is not correct casting. If total size is > 2GB, e.g. overflows 32-bit int on system with 32-bit ints, then the result of the multiplication will produce an out of range value truncated to 32-bits. Then the truncated value will be upcast to 64-bit (if size_t is 64-bits). This is too late, truncation is already done. If you want to handle image pixel size > 2GB, you must cast to 64-bit before multiplying so that the product is 64-bits. E.g., (size_t)width * height * depth. That will upcast width to size_t (probably 64-bit), and then standard C rules will promote the other operands to the multiplication to match it.

[xyzzy42]

Actually, this does get the frame size right.

But it re-encode everything into a non-packed format. It was previously possible to encode packed data directly without upconverting it into a larger format. It's only necessary to upconvert with the functions that perform image modification operations that do not support packed pixels. Sixel encoding does support packed pixels, do it's not necessary to do. But the change to sixel_encoder_encode_bytes() will upconvert everthing.

Because there are no tests for memory usage or performance, the AI it does understand them at all. It will happily write code that is very wasteful.

sixel_encoder_compute_frame_size() looks like it does nothing different than sixel_encoder_compute_frame_size().

The python interface is still broken, as sixel_helper_compute_depth() is still used there to compute expected buffer sizes.

[saitoha]

Thanks for the additional feedback. I understand there are two separate issues here:

1. In the Python interface, len(buf) < width * height * depth does not account for packed formats, so the validation is stricter than necessary.

2. In the develop-branch fix(816212b), packed-input upconversion currently causes extra allocation/copy overhead.

For point (2), I do not think upconversion itself is inherently wasteful in the current pipeline. As implemented today, there is no end-to-end path that keeps G1/G2/G4/PAL1/PAL2/PAL4 packed all the way through processing; packed inputs are normalized before operations that require byte-per-pixel access. In that sense, the main question is where normalization happens, not whether it happens.

That said, the current develop-path does an ownership copy first (malloc + memcpy), then allocates again for normalized data and performs another full data pass. I agree this extra intermediate allocation/copy path is wasteful and should be removed.