Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX: set max row count to avoid iterating off the end of the data buffer #22

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ZLLentz
Copy link
Member

@ZLLentz ZLLentz commented Dec 18, 2024

For reviewers

I'm not quite ready to deploy this but I'd like opinions from people who have experience writing C++.
I'd particularly like tips or ideas for doing this more intelligently.

Description

There's a rare bug that deals with improperly-configured cameras. I suspect it also could happen as a race condition on a camera resize.

This bug manifests itself when the "count" of the epics array is shorter than the total number of pixels. In some cases this results in garbage data making it into the image, in other cases this results in a segfault.

This update aims to add minimal processing overhead to the new array processor while stopping it from iterating past the end of the data buffer. It does this by using the previously unused "count" long from pyca which tells us how many data elements there are, and doing a quick one-time calculation of the max row we can get to without exhausting the buffer. Then we sub this number in for the row count in the iteration and stop right after the final row.

This works in my toy example I cooked up and it doesn't crash the GUI, but before merging I'd need to intentionally misconfigure a camera to check what the behavior ends up being.

Copy link

@baljamal baljamal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like your solutions. in addition I have suggestions that I think it will limit segfualt problems:

  1. using -fsanitize compile flag to make it easier to detect such problems
  2. replace c-style arrays with std::array with exception handling

janeliu-slac
janeliu-slac previously approved these changes Dec 20, 2024
Comment on lines +352 to +360
int _max_row(ImageBuffer *imageBuffer, long count)
{
if (imageBuffer->imgwidth > 0) {
return std::min(imageBuffer->imgheight, (int) std::floor(count / imageBuffer->imgwidth));
} else {
return 0;
}
}

Copy link

@janeliu-slac janeliu-slac Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is minor but if using a modern C++ approach I think a smart pointer is considered best practice. Raw pointers should be used only when performance/memory management are critical.

If only one pointer for ImageBuffer is needed, std::unique_ptr would work.

#include <memory> 

int _max_row(unique_ptr<ImageBuffer> imageBuffer, long count)
{
    if (imageBuffer->imgwidth > 0) {
        return std::min(imageBuffer->imgheight, (int) std::floor(count / imageBuffer->imgwidth));
    } else {
        return 0;
    }
}

Regarding the buffer overflow problem--if the program uses std::vector, std::array or other dynamically sized containers similar to a python list maybe this issue can be avoided. Raw arrays is probably one of the major reasons the world moved away from C.

@ZLLentz
Copy link
Member Author

ZLLentz commented Dec 20, 2024

Thank you Basil and Jane- I'll use your comments and guidance as a starting point for some C++ learning for me in the new year. If we can make this less likely to crash with bad inputs it's definitely worthwhile.

@@ -1991,6 +1991,22 @@ def connectCamera(self, sCameraPv, index, sNotifyPv=None):
# Get camera configuration
self.getConfig()

# Check the expected size against the count to generate warnings
first_image_count = len(self.camera.value)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to future me: does this work with color cameras? Need to check

@janeliu-slac janeliu-slac dismissed their stale review December 20, 2024 00:59

Just realized that this is still a work in progress.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants