[PR] copy buffered data not the GByteArray struct in c_glib read_slow [thrift]

["dxbjavid (via GitHub)" <gi...@apache.org>]

dxbjavid opened a new pull request, #3607:
URL: https://github.com/apache/thrift/pull/3607

   thrift_framed_transport_read_slow copies the bytes left over from the previous frame with memcpy(buf, t->r_buf, t->r_buf->len), which passes the GByteArray structure as the source instead of t->r_buf->data. When a read crosses a frame boundary with more than a few bytes still buffered (the peer controls the frame sizes), the structure's own pointer and length fields come back in place of the data and the copy runs past the end of the small GByteArray allocation, so adjacent heap memory leaks into the decoded message. The buffered transport carries the same mistake in its leftover path; both now read from t->r_buf->data like the other two copies in each file, and a cross-frame regression test driven through a memory buffer exercises the path.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] copy buffered data not the GByteArray struct in c_glib read_slow [thrift]

["dxbjavid (via GitHub)" <gi...@apache.org>]

dxbjavid commented on PR #3607:
URL: https://github.com/apache/thrift/pull/3607#issuecomment-4843153368

   Thank you for your time.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] copy buffered data not the GByteArray struct in c_glib read_slow [thrift]

["Jens-G (via GitHub)" <gi...@apache.org>]

Jens-G commented on PR #3607:
URL: https://github.com/apache/thrift/pull/3607#issuecomment-4813638004

   ### Code review
   
   Found 2 issues:
   
   1. `testframedtransport_LDADD` in `Makefile.am` does not include `thrift_memory_buffer.o`, but the new `test_read_across_frames` uses `THRIFT_TYPE_MEMORY_BUFFER`. `CMakeLists.txt` masks this by linking the full `thrift_c_glib` library, but autotools builds will fail at link time with undefined references. Other tests in this directory that use `ThriftMemoryBuffer` include the `.c` file directly (e.g. `#include "../src/thrift/c_glib/transport/thrift_memory_buffer.c"`).
   
   https://github.com/apache/thrift/blob/f20a320c2c6ba061e918751210267b7318635ff1/lib/c_glib/test/Makefile.am#L156-L163
   
   2. The fix in `thrift_buffered_transport_read_slow` has no regression test. The equivalent fix in `thrift_framed_transport_read_slow` gets `test_read_across_frames`, but `testbufferedtransport.c` has nothing that exercises the `have > 0` branch. Per CONTRIBUTING.md, tests are required for every submitted change.
   
   https://github.com/apache/thrift/blob/f20a320c2c6ba061e918751210267b7318635ff1/lib/c_glib/src/thrift/c_glib/transport/thrift_buffered_transport.c#L93-L100
   
   🤖 Generated with [Claude Code](https://claude.ai/code)
   
   <sub>- If this code review was useful, please react with 👍. Otherwise, react with 👎.</sub>


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] copy buffered data not the GByteArray struct in c_glib read_slow [thrift]

["dxbjavid (via GitHub)" <gi...@apache.org>]

dxbjavid commented on PR #3607:
URL: https://github.com/apache/thrift/pull/3607#issuecomment-4815200885

   both fixed.
   
   for the link issue i went with adding thrift_memory_buffer.o to testframedtransport_LDADD (and testbufferedtransport_LDADD) rather than including the .c directly. the readcheck test can include thrift_memory_buffer.c because it's the only transport source it pulls in, but these two files already #include thrift_framed_transport.c / thrift_buffered_transport.c, and a second transport .c in the same translation unit collides on the file-local PROP_0 enum. linking the object keeps it in its own unit and the autotools build resolves cleanly.
   
   for the missing coverage i added ReadAcrossBuffer to testbufferedtransport.c. the buffered read path never pre-buffers on its own, so to reach the have > 0 branch the test seeds the read buffer with 96 leftover bytes (well past sizeof(GByteArray)) and then reads across it through a memory buffer. under asan it reports a heap-buffer-overflow at thrift_buffered_transport.c:96 against the old memcpy and passes with the data-member copy.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] copy buffered data not the GByteArray struct in c_glib read_slow [thrift]

["Jens-G (via GitHub)" <gi...@apache.org>]

Jens-G closed pull request #3607: copy buffered data not the GByteArray struct in c_glib read_slow
URL: https://github.com/apache/thrift/pull/3607


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org