Bringing light object to high resolution camera results in overflkow error

[nickolay168]

Repro: Use Video-capable tox client (I used qTox); Start a video call; Show shiny object to the camera objective (I used QR code on the smartphone screen).

The client will stop unexpectedly and if it will be launched from the console the "Overflow error" will be output. Please see the PR with more description and fix: https://github.com/TokTok/c-toxcore/pull/2766

[zoff99]

@nickolay168 what version of qTox did you use? and on what OS? can you include a stack trace? or the source code line where the crash happens?

[nickolay168]

Sure! qTox version: 1.17.6, however I am working on my fork, containing some fixes. OS: Fedora 40 The failure happens on line 839 Unfortunately stack trace becomes cryptic during failure, however, we can see it during debug

rtp_send_data() at rtp.c:839 0x7ffff532ba4d     
send_frames() at toxav.c:902 0x7ffff532e505     
toxav_video_send_frame() at toxav.c:1,020 0x7ffff532e9d3        
CoreAV::sendCallVideo() at coreav.cpp:432 0x46d0ef      
operator() at toxcall.cpp:148 0x48f225  
QtPrivate::FunctorCall<QtPrivate::IndexesList<0>, QtPrivate::List<std::shared_ptr<VideoFrame> >, void, ToxFriendCall::ToxFriendCall(uint32_t, bool, CoreAV&, IAudioControl&, CameraSource&)::<lambda(std::shared_ptr<VideoFrame>)> >::call at qobjectdefs_impl.h:146 0x492c2b     
QtPrivate::Functor<ToxFriendCall::ToxFriendCall(uint32_t, bool, CoreAV&, IAudioControl&, CameraSource&)::<lambda(std::shared_ptr<VideoFrame>)>, 1>::call<QtPrivate::List<std::shared_ptr<VideoFrame> >, void> at qobjectdefs_impl.h:256 0x492873  
QtPrivate::QFunctorSlotObject<ToxFriendCall::ToxFriendCall(uint32_t, bool, CoreAV&, IAudioControl&, CameraSource&)::<lambda(std::shared_ptr<VideoFrame>)>, 1, QtPrivate::List<std::shared_ptr<VideoFrame> >, void>::impl at qobjectdefs_impl.h:443 0x492263       
void doActivate<false>() at 0x7ffff3aebf1e      
VideoSource::frameAvailable() at moc_videosource.cpp:146 0x55fe96       
operator() at camerasource.cpp:428 0x4d43f3     
CameraSource::stream() at camerasource.cpp:447 0x4d4497 
std::__invoke_impl<void, void (CameraSource::*&)(), CameraSource*&> at invoke.h:74 0x4d5b4b     
std::__invoke<void (CameraSource::*&)(), CameraSource*&> at invoke.h:96 0x4d5a05        
std::_Bind<void (CameraSource::*(CameraSource*))()>::__call<void, , 0ul> at functional:513 0x4d5925     
std::_Bind<void (CameraSource::*(CameraSource*))()>::operator()<, void> at functional:598 0x4d5889      
QtConcurrent::StoredFunctorCall0<void, std::_Bind<void (CameraSource::*(CameraSource*))()> >::runFunctor at qtconcurrentstoredfunctioncall.h:70 0x4d5807        
QtConcurrent::RunFunctionTask<void>::run() at qtconcurrentrunbase.h:142 0x4d48cf        
QThreadPoolThread::run() at 0x7ffff38f2e8b      
QThreadPrivate::start() at 0x7ffff38efbc6       
start_thread() at 0x7ffff32a66d7        
clone3() at 0x7ffff332a60c

The issue is that length may be super big, resulting in the unsuccessful memory allocation on line 809. But the effect will be seen on line 839.

P.S. Thank you very much for toxcore project, it is really great!

[zoff99]

can you compile qTox with ASAN? VLA does alloca() on the stack. can you also print the size you are actually allocating? if you cut length to 1 single packet you will get the "old" video bug again.

https://github.com/TokTok/c-toxcore/issues/620

[zoff99]

also qTox enhanced is already forked and has NGC and other stuff. but still i would not recommend using any version of qTox since it has lots of end-of-life components and bugs.

use either https://github.com/Zoxcore/trifa_material or https://github.com/JFreegman/toxic

[Green-Sky]

I took a look at this, since this is happening to me too and came up with this simple patch:

index 8ad4ce670e9..49c91a02ce9 100644
--- a/external/toxcore/c-toxcore/toxav/rtp.c
+++ b/external/toxcore/c-toxcore/toxav/rtp.c
@@ -805,7 +805,7 @@ int rtp_send_data(RTPSession *session, const uint8_t *data, uint32_t length,
         header.flags |= RTP_KEY_FRAME;
     }

-    const uint16_t rdata_size = length + RTP_HEADER_SIZE + 1;
+    const uint16_t rdata_size = min_u32(length + RTP_HEADER_SIZE + 1, MAX_CRYPTO_DATA_SIZE);
     VLA(uint8_t, rdata, rdata_size);
     memset(rdata, 0, rdata_size);
     rdata[0] = session->payload_type;  // packet id == payload_type

rdata_size here is 16bit, which gets cast from 32bit length, which can be much larger than the max representable of 16bit. BUT, rdata never contains more than a single packet, so why even try to allocate so much...

@nickolay168 another issue toxav currently has, is the fact that while the api states the video bitrate is kbits, it is actually mbits, which is the main reason video frames get this large AND look ugly. The minimum, 1mbit/s, is already too much for simple webcams in most cases. Try hardcoding it to 1 (or anything <10) in your qtox fork. We are aware of this issue and it will be fixed eventually.

[zoff99]

ah and now i see why its not happening in my fork:

https://github.com/zoff99/c-toxcore/blob/bd9dc518c317e5032fe92a9e44c70ce90e73ea65/toxav/rtp.c#L119

    VLA(uint8_t, rdata, length + sizeof(struct RTPHeader) + 1);
    memset(rdata, 0, SIZEOF_VLA(rdata));

someone must have refactored this part and caused the issue.

[zoff99]

and here it is:

https://github.com/TokTok/c-toxcore/commit/5c093c4888f61368cab2f31ad6e5023fac7cd091

https://github.com/TokTok/c-toxcore/commit/5c093c4888f61368cab2f31ad6e5023fac7cd091#diff-e55d4279958d7ba49ea2c592cb263c88574b3c0de72dcf8447c2acb36a59e22fR809

[nickolay168]

I took a look at this, since this is happening to me too and came up with this simple patch:

index 8ad4ce670e9..49c91a02ce9 100644
--- a/external/toxcore/c-toxcore/toxav/rtp.c
+++ b/external/toxcore/c-toxcore/toxav/rtp.c
@@ -805,7 +805,7 @@ int rtp_send_data(RTPSession *session, const uint8_t *data, uint32_t length,
         header.flags |= RTP_KEY_FRAME;
     }

-    const uint16_t rdata_size = length + RTP_HEADER_SIZE + 1;
+    const uint16_t rdata_size = min_u32(length + RTP_HEADER_SIZE + 1, MAX_CRYPTO_DATA_SIZE);
     VLA(uint8_t, rdata, rdata_size);
     memset(rdata, 0, rdata_size);
     rdata[0] = session->payload_type;  // packet id == payload_type

rdata_size here is 16bit, which gets cast from 32bit length, which can be much larger than the max representable of 16bit. BUT, rdata never contains more than a single packet, so why even try to allocate so much...

@nickolay168 another issue toxav currently has, is the fact that while the api states the video bitrate is kbits, it is actually mbits, which is the main reason video frames get this large AND look ugly. The minimum, 1mbit/s, is already too much for simple webcams in most cases. Try hardcoding it to 1 (or anything <10) in your qtox fork. We are aware of this issue and it will be fixed eventually.

Sure, I will work on this, thank you! Another question, is that we can get frame bigger then MAX_CRYPTO_DATA_SIZE, in this case we were sending it in multiple packages. After the fix we will not use the rest of the data. Could it be a problem?

[nickolay168]

also qTox enhanced is already forked and has NGC and other stuff. but still i would not recommend using any version of qTox since it has lots of end-of-life components and bugs.

use either https://github.com/Zoxcore/trifa_material or https://github.com/JFreegman/toxic

Yes, Trifa is great, I am using it on the smartphone, but there is a big problem with desktop clients, all of them seems to be not supported. I am trying to keep qTox live...

[Green-Sky]

Sure, I will work on this, thank you! Another question, is that we can get frame bigger then MAX_CRYPTO_DATA_SIZE, in this case we were sending it in multiple packages. After the fix we will not use the rest of the data. Could it be a problem?

What do you mean by "will not use the rest of the data"

Yes, Trifa is great, I am using it on the smartphone, but there is a big problem with desktop clients, all of them seems to be not supported. I am trying to keep qTox live...

If you feel experimental, you can give tomato a spin. I use it daily, so it has some stability, but there are features missing you might expect form clients like qtox.

[Green-Sky]

@nickolay168 btw, feel free to join us in the toktok ngc: 360497da684bce2a500c1af9b3a5ce949bbb9f6fb1f91589806fb04ca039e313 ... with a client that supports it (toxic, trifa_material, tomato or you could extract the ngc patches from qtox_enhanced)

[nurupo]

Another question, is that we can get frame bigger then MAX_CRYPTO_DATA_SIZE, in this case we were sending it in multiple packages. After the fix we will not use the rest of the data.

All of the data is still being sent, in multiple packets if needed, even with the fix applied.

[nickolay168]

Yes, that is right, I have looked into wrong place.

[nickolay168]

@nickolay168 btw, feel free to join us in the toktok ngc: 360497da684bce2a500c1af9b3a5ce949bbb9f6fb1f91589806fb04ca039e313 ... with a client that supports it (toxic, trifa_material, tomato or you could extract the ngc patches from qtox_enhanced)

Thank you!