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

Viewer blindly trusts tensor dimensions and crashes if buffer is too small #3927

Closed
Wumpf opened this issue Oct 19, 2023 · 1 comment · Fixed by #4005
Closed

Viewer blindly trusts tensor dimensions and crashes if buffer is too small #3927

Wumpf opened this issue Oct 19, 2023 · 1 comment · Fixed by #4005
Assignees
Labels
🪳 bug Something isn't working 💣 crash crash, deadlock/freeze, do-no-start 🔺 re_renderer affects re_renderer itself 📺 re_viewer affects re_viewer itself
Milestone

Comments

@Wumpf
Copy link
Member

Wumpf commented Oct 19, 2023

Tensors/Images take a tensor buffer. Tensor buffers have both a buffer size and a dimension. It's trivial in Rust and C++ to give a dimension that is larger than the actual buffer (it is possible, in Python if you supply tensor dimensions manually, but kinda hard). There's nothing right now that validates this.

The viewer should truncate and warn!

C++ repro case:

   std::string image_path = "rerun-logo.png";
    cv::Mat img = imread(image_path, cv::IMREAD_COLOR);
    if (img.empty()) {
        std::cout << "Could not read the image: " << image_path << std::endl;
        return 1;
    }

    // Log image to Rerun
    cv::cvtColor(img, img, cv::COLOR_BGR2RGB); // Rerun expects RGB format
    // NOTE currently we need to construct a vector to log an image, this will change in the future
    //  see https://github.com/rerun-io/rerun/issues/3794
    std::vector<uint8_t> img_vec(img.total() * img.channels());
    img_vec.assign(img.data, img.data + img.total() * img.channels());
    rec.log(
        "image",
        rerun::Image(
            {static_cast<size_t>(img.rows),
             static_cast<size_t>(img.cols),
             static_cast<size_t>(img.channels() + 1)}, // +1 makes this wrong
            std::move(img_vec)
        )
    );

Curiously enough the crash only happens once we try upload textures to the gpu (would have expected to crash earlier):

[2023-10-19T08:41:32Z ERROR wgpu::backend::direct] Handling wgpu errors as fatal by default
thread 'ThreadId(1)' panicked at 'wgpu error: Validation Error

Caused by:
    In Queue::write_texture
    Copy of 0..1048576 would end up overrunning the bounds of the Source buffer of size 786432

'
wgpu-0.17.0/src/backend/direct.rs:3056
stack backtrace:
   6: core::panicking::panic_fmt
             at core/src/panicking.rs:67:14
   7: core::ops::function::Fn::call
@Wumpf Wumpf added 🪳 bug Something isn't working 🦀 Rust API Rust logging API 📺 re_viewer affects re_viewer itself 🌊 C++ API C/C++ API specific labels Oct 19, 2023
@Wumpf Wumpf added this to the 0.11 C++ polish milestone Oct 19, 2023
@Wumpf Wumpf added the 🔺 re_renderer affects re_renderer itself label Oct 19, 2023
@Wumpf
Copy link
Member Author

Wumpf commented Oct 19, 2023

There should be several layers of catching this: The viewer warning and working around, the renderer failing gracefully.

@emilk emilk added 💣 crash crash, deadlock/freeze, do-no-start and removed 🦀 Rust API Rust logging API 🌊 C++ API C/C++ API specific labels Oct 23, 2023
@emilk emilk self-assigned this Oct 25, 2023
emilk added a commit that referenced this issue Oct 26, 2023
#4005)

### What
* Closes #3927

wgpu does no validation right away, but defer it to a background thread,
leading to lack of good error messages, and sometimes panics.

So we need to do the best we can up-front!

I tested this with:

```diff
--- a/docs/code-examples/image_simple.cpp
+++ b/docs/code-examples/image_simple.cpp
@@ -21,5 +21,5 @@ int main() {
         }
     }
 
-    rec.log("image", rerun::Image({HEIGHT, WIDTH, 3}, std::move(data)));
+    rec.log("image", rerun::Image({HEIGHT + 100, WIDTH, 3}, std::move(data)));
 }
```

Result:

> [2023-10-25T13:10:56Z ERROR re_space_view_spatial::parts::images]
Failed to create texture for "image": Wrong number of bytes in the
texture data buffer - expected 300x300x4=360000, got 240000



### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested [demo.rerun.io](https://demo.rerun.io/pr/4005) (if
applicable)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/4005)
- [Docs
preview](https://rerun.io/preview/d94937b6991a03e998e9106bdf51e7390263b4ff/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/d94937b6991a03e998e9106bdf51e7390263b4ff/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://ref.rerun.io/dev/bench/)
- [Wasm size tracking](https://ref.rerun.io/dev/sizes/)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪳 bug Something isn't working 💣 crash crash, deadlock/freeze, do-no-start 🔺 re_renderer affects re_renderer itself 📺 re_viewer affects re_viewer itself
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants