Segmentation fault for response back sending image to client

[ericjansen]

Hi @kentonv

I thank you for the way you shared for the basic way to send image. I tried to use your way and so far I am successful to send the image data from client to server.
But I am unable to return back the image from server to client. I received segmentation fault for promise.then in the client part
The schema looks like this:

struct Image {
  size @0 :Int32;
  data @1 :Data;
}

interface ImageTransfer {
  transferImage @0 (image :Image) -> (image :Image);
}

Here is the server code:

class ImageTransferImpl final : public ImageTransfer::Server
{
public:
    kj::Promise<void> transferImage(TransferImageContext context) override
    {
        auto image = context.getParams().getImage();

        auto size = image.getSize();
        auto img = image.getData();

        std::cout << "Received image with size: " << size << " bytes\n";
        cv::Mat m(1, size, CV_8UC1, const_cast<void *>(static_cast<const void *>(img.begin())));
        cv::Mat receivedImage = cv::imdecode(m, cv::IMREAD_COLOR);
        cv::Mat grayscaleImg;
        cv::cvtColor(receivedImage, grayscaleImg, cv::COLOR_BGR2GRAY);

        std::vector<uchar> data;
        cv::imencode(".jpg", grayscaleImg, data);
        response.initImage().setSize(data.size());

        auto imgBuilder = response.initImage();
        auto dataBuilder = imgBuilder.initData(data.size());
        std::memcpy(dataBuilder.begin(), data.data(), data.size());

        response.setImage(imgBuilder);
        return kj::READY_NOW;
    }
};

and the client code:

    auto msg = message.initRoot<ImageTransfer::TransferImageParams>();

    std::ifstream file("a.jpg", std::ios::binary | std::ios::ate);
    if (!file.is_open()) {
        std::cerr << "Failed to open image file.\n";
        return 1;
    }
    std::streamsize size = file.tellg();
    file.seekg(0, std::ios::beg);
    //auto data = request.initData(size);
    auto data = msg.getImage().initData(size);
    file.read(reinterpret_cast<char*>(data.begin()), size);
    msg.getImage().setSize(size);

    std::cout << "Size: " << size << "\n";

    capnp::TwoPartyClient client(*conn);
    ImageTransfer::Client img = client.bootstrap().castAs<ImageTransfer>();

    auto request = img.transferImageRequest();
    request.setImage(msg.getImage());

    std::cout << "Sending request to server\n";
    auto promise = request.send();
    std::cout << "Request sent\n";

    auto readPromise = promise.then([](auto response) {
        auto imageResponse = response.getImage();
        std::cout << "Received image from server with size: " << imageResponse.getSize() << " bytes\n";
        return imageResponse;
    });

The return from the server looks like this:

$ ./server 
Listening on port 12345

The return from the client looks like this:

$ ./client
Size: 60731
Sending request to server
Request sent
Segmentation fault: 11

Please let me know if I miss something.

Thank you and kind regards,
Eric

[kentonv]

On this line:

auto imageResponse = response.getImage();

Note that imageResponse is type Image::Reader. A struct's Reader type (as well as Builder) behaves like a pointer: it does not own the value it points at. A Reader points into the underlying message buffer. That buffer is owned by response. At the end of this function, you are doing return imageResponse. This is returning a pointer. However, response is destroyed at the end of the function. Therefore, the returned value becomes a dangling pointer. When you access it, you may get a segmentation fault.

It looks like your code listing cuts off before readPromise is actually used, so it's hard for me to suggest what you should do instead here, but in short you need to make sure the response object stays alive until the content has been fully consumed.

---

A couple unrelated notes on your code:

response.setImage(imgBuilder);

This line is redundant. imgBuilder already points at response.getImage(), so you are setting the value to itself. You should delete this line.

struct Image {
  size @0 :Int32;
  data @1 :Data;
}

The size here is redundant. Data already tracks its own size -- you can do image.getData().size(). I think you should remove the size field, because otherwise it could lead to bugs if it doesn't actually match the size of data.

[ericjansen]

Hi @jclee

Thank you for your guidance. It's indeed without segmentation fault now.
client.cpp

int main() {
    auto io = kj::setupAsyncIo();

    auto &waitScope = io.waitScope;
    kj::Network &network = io.provider->getNetwork();
    kj::Own<kj::NetworkAddress> addr = network.parseAddress("127.0.0.1:12000").wait(io.waitScope);
    kj::Own<kj::AsyncIoStream> conn = addr->connect().wait(io.waitScope);

    capnp::MallocMessageBuilder message;
    auto msg = message.initRoot<Image::TransferImageParams>();

    std::ifstream file("a.jpg", std::ios::binary | std::ios::ate);
    if (!file.is_open()) {
        std::cerr << "Failed to open image file.\n";
        return 1;
    }
    std::streamsize size = file.tellg();
    file.seekg(0, std::ios::beg);
    auto data = msg.initImage(size);
    file.read(reinterpret_cast<char*>(data.begin()), data.size());
    std::cout << "Size: " << size << "\n";

    capnp::TwoPartyClient client(*conn);
    Image::Client img = client.bootstrap().castAs<Image>();

    auto request = img.transferImageRequest();
    request.setImage(data);

    std::cout << "Sending request to server\n";
    auto promise = request.send();
    std::cout << "Request sent\n";

    auto readPromise = promise.then([](auto response) {
        auto imageResponse = response.getImage();
        return imageResponse;
    });

    if (file) {
        file.close();
    }

    auto response = readPromise.wait(waitScope);
    std::ofstream wfile("received.jpg", std::ios::binary);
    if (wfile.is_open()) {
         wfile.write(reinterpret_cast<const char*>(response.begin()), response.size());
        wfile.close();
        std::cout << "saved to received_image.jpg\n";
    } else {
        std::cout << "Failed to save to received_image.jpg\n";
        return 1;
    }
    
    return 0;
}

server.cpp is similar with the previous example.

But I don't know why the image received from wfile is not a valid image. Maybe the way I save the image is wrong.

Thank you and kind regards,
Eric

[ericjansen]

It seems I need to provide height and width of the image to be able to receive properly.
Could you advise me any ideas?
Thank you

[kentonv]

Eric, I'm sorry, but we aren't able to help you with every step of designing and debugging your code. Cap'n Proto is an open source project provided for free, which means it is up to you to learn how to use it by looking at the docs, examples, and source code. I'm happy to answer a few questions when the answer can't be found by any other means, but I do this only on a volunteer basis.

[ericjansen]

No worries @kentonv , your guidelines are very helpful already. Thank you.

[jclee]

@ericjansen FWIW, it looks like your new code has the same problem as the original code; readPromise is returning a kj::ArrayPtr to memory owned by the response parameter, which goes out of scope before the kj::ArrayPtr gets used. As earlier, you'd need to "make sure the response object stays alive until the content has been fully consumed", either by moving the file writing code into the same scope as the reponse object or by returning a value from the promise that maintains ownership of the result memory.