Add range check to NewDecoder

[alexrsagen]

The current code produces the following error on an empty buffer. The added range check prevents it from panicking, instead returning an error.

panic: runtime error: index out of range

goroutine 1 [running]:
github.com/discordapp/lilliput.newOpenCVDecoder(0xc4200e6200, 0x0, 0x200, 0x0, 0x0, 0xc420130040)
        /home/alexrsagen/go/src/github.com/discordapp/lilliput/opencv.go:234 +0x16a
github.com/discordapp/lilliput.NewDecoder(0xc4200e6200, 0x0, 0x200, 0x0, 0x200, 0x0, 0x0)
        /home/alexrsagen/go/src/github.com/discordapp/lilliput/lilliput.go:67 +0x72

[alexrsagen]

Note that the example code does not include a range check, so many people using this library might not be either, causing potential DoS vulnerabilities due to panic.

Adding the check to the library instead of requiring users to do it prevents any such vulnerabilities.

[brian-armstrong-discord]

Hi @alexrsagen,

Thanks for the PR! There's definitely some sharp edges here that haven't been sanded down.

I don't think this case warrants a new error. In all other cases, a buffer that doesn't contain an image returns ErrInvalidImage. It makes sense that it should here, too, which was the intended behavior anyway. I think the other new error (for Fit()) looks good though. Would you mind changing the first error and pushing again?

[alexrsagen]

Hey, thanks for the review @brian-armstrong-discord!

I've made the requested change. I thought of the same initially, but got hung up on wording ("too small to hold image"). I mentioned this to @SpencerSharkey and IIRC he also thought this seemed weird for this case. Anyway that's just semantics, not really important for the end result. I agree the error message should be the same because of its nature.

I think the same range check should also be added to NewEncoder as it doesn't check the size of the destination buffer. Even though the user is responsible for allocating a buffer with len > 0, it might be unclear whether the library will simply make([]byte, len) a new buffer given an empty []byte. Let me know if you agree!

[alexrsagen]

I also noticed NewDecoder ignores any error message from newOpenCVDecoder. I see there are two possible error cases from it, and assume ErrInvalidImage should make it attempt another decoder. Instead of ignoring the other possible ErrBufTooSmall, perhaps it should only ignore the error ErrInvalidImage, and return on any other error?

[brian-armstrong-discord]

Hi @alexrsagen,

Just to be clear, the error we'd want to use here is ErrInvalidImage in that case, not ErrBufTooSmall. That's because all the other branches in that function already return ErrInvalidImage (and it feels semantically correct here). If you wouldn't mind, please update the PR again with this error.

The ErrBufTooSmall in the opencv decoder is sort of a strange thing. It might actually be better if it were a panic since it would likely indicate something is fundamentally broken in the library. It's not an error that you can actually encounter in runtime, as far as I can tell.

To give more context, that error occurs when opencv_mat_create_from_data() is given a backing buffer that's too small for the cv::Mat object it's trying to create. cv::Mat is just a wrapper on some kind of uint8_t[] that helps with pointer arithmetic. For example, if I asked it for a 2x2 array of some one byte object, I would need to give it a 4-byte array (specified by the data_len argument).

In the case of the opencv decoder, we're specifying a 1-dimensional array on top of the buffer. An array of size 1 x len(buf) should always be backed by a buffer of len len(buf) -- it's sort of tautological. I think I'm convincing myself now that this really should be a panic and not an error. It's not something the user can cause.

To reiterate, the only error that should actually be produced by the decoder creation is ErrInvalidImage. And indeed, the genericized decoder creator ignores errors because it's trying each type until it finds a match.

[alexrsagen]

Thanks for the explanation of that error case. Indeed it does seem that is one of those "universe correctness" tests which is only useful for detecting cosmic rays corrupting your memory :)

Though it certainly doesn't hurt to have the check in place, it might not be neccessary like you say. If you decide a panic would be better, i believe it should panic on its own (nil pointer dereference) in the error case, without needing a direct call to panic().

What are your thoughts on adding a zero-length check to NewEncoder for dst []byte ?

[brian-armstrong-discord]

As a reminder, please change https://github.com/discordapp/lilliput/pull/32/files#diff-7679cb56f5ca76ab584bbc8032a4892eR65 to ErrInvalidImage so I can merge :)

Regarding the encoder - maybe. I'd like to handle that in another PR. There's a related bit that probably needs revisiting at https://github.com/discordapp/lilliput/blob/master/opencv.go#L346 (scary XXXs!) OpenCV will just build a new buffer if you give it one that's too small, which sort of disrupts various assumptions in this library (and how people will use it). So I'd like to think about it some more.

[alexrsagen]

Updated the error type!

I would recommend creating a memory release function in opencv.cpp something along these lines

void opencv_mat_release(const opencv_mat mat) {
    auto cvMat = static_cast<const cv::Mat *>(mat);
    cvMat->release();
}

and calling it at the point of scary XXXs in the code.

As cv::Mat should be the "owner" of the new buffer, it should decallocate its memory upon cv::Mat::release() (source). Note that this is called upon cv::Mat destruction, but it is not dangerous to manually call the release function before destruction. I am also not sure whether the destructor will currently ever get called.

You can verify the memory deallocation by checking the cv::Mat::data pointer after calling cv::Mat::release(). It should be nullptr.

[brian-armstrong-discord]

I'm not sure. Like I said, I'd like to think about it some more. The trickiness here regards user expectations. The user gave us a buffer to write into, and instead we allocated a new buffer and wrote into that one. It's not good that it happened at all, but throwing away the result doesn't necessarily feel right.

I think the right option here might be to further fork opencv and disable this behavior, since it's nonsensical anyway.

Anyway, thanks for the patch.