THRIFT-5237 Implement MAX_MESSAGE_SIZE and consolidate limits into a TConfiguration class (c_glib) #2208

zeshuai007 · 2020-07-29T03:30:50Z

TConfiguration class (c_glib)

Did you create an Apache Jira ticket? (not required for trivial changes)
If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
Did you squash your changes to a single commit? (not required, but preferred)
Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

Jens-G · 2020-07-29T07:12:44Z

I'd recommend to ask on the mailing list to find someone to review. I can have a look but my c_glib knowledge is far from being an expert, so a second opinion would be sort of required anyway.

Jens-G

Looks good so far, except a few minorish things I'd like you to fix or modify.

Jens-G · 2020-08-05T10:01:40Z

lib/c_glib/src/thrift/c_glib/protocol/thrift_binary_protocol.c

    return -1;
  }

+  if(FALSE == ttc->checkReadBytesAvailable(THRIFT_TRANSPORT(tp->transport), 


if( FALSE == ...) - we can do better, can we?

Jens-G · 2020-08-05T10:02:13Z

lib/c_glib/src/thrift/c_glib/protocol/thrift_binary_protocol.c

    return -1;
  }

+  if(FALSE == ttc->checkReadBytesAvailable(THRIFT_TRANSPORT(tp->transport), 


Jens-G · 2020-08-05T10:02:34Z

lib/c_glib/src/thrift/c_glib/protocol/thrift_compact_protocol.c

  }

+
+  if(FALSE == ttc->checkReadBytesAvailable(THRIFT_TRANSPORT (tp->transport), 


again, and there are a few more below

Jens-G · 2020-08-05T10:05:08Z

lib/c_glib/src/thrift/c_glib/transport/thrift_buffered_transport.c

+				  "Set the size of the remaining message",
+				  0, /* min */
+                                  G_MAXINT32, /* max */
+				  10485760, /* default by construct */


can we put that 10485760 into some constant and give it a meaningful name?

Jens-G · 2020-08-05T10:05:37Z

lib/c_glib/src/thrift/c_glib/transport/thrift_buffered_transport.c

+				  "Set the size of the know message",
+				  0, /* min */
+                                  G_MAXINT32, /* max */
+				  10485760, /* default by construct */


same, plus some more below

Jens-G · 2020-08-05T10:07:35Z

lib/c_glib/src/thrift/c_glib/transport/thrift_server_socket.c

+      transport->configuration = g_value_dup_object (value);
+      if(transport->configuration != NULL)
+      {
+	transport->remainingMessageSize_ = transport->configuration->maxMessageSize_;


minor: indent?

Jens-G · 2020-08-05T10:07:41Z

lib/c_glib/src/thrift/c_glib/transport/thrift_server_transport.c

+          transport->configuration = g_value_get_object (value);
+	  if(transport->configuration != NULL)
+	  {
+              transport->remainingMessageSize_ = transport->configuration->maxMessageSize_;


minor: indent?

Jens-G · 2020-08-05T10:10:55Z

lib/c_glib/src/thrift/c_glib/transport/thrift_transport.c

+    {
+	if(tt->configuration != NULL)
+	{
+            tt->knowMessageSize_ = tt->configuration->maxMessageSize_;


minor: indent?

Jens-G · 2020-08-05T10:11:13Z

lib/c_glib/src/thrift/c_glib/transport/thrift_transport.c

+    else
+    {
+        tt->remainingMessageSize_ = 0;
+	g_set_error(error,


Jens-G · 2020-08-05T10:11:57Z

lib/c_glib/src/thrift/c_glib/transport/thrift_transport.c

+
+  param_spec = g_param_spec_object ("configuration",
+		                    "configuration (construct)",
+				    "Thtift Configuration",


"Thtift Configuration" => "Thrift Configuration"

zeshuai007 · 2020-08-06T09:32:47Z

Done.Please review again.

…TConfiguration class (c_glib)

…TConfiguration class Client: c_glib Patch: Zezeng Wang This closes apache#2208

cyril867 · 2021-03-17T09:44:01Z

@zeshuai007 hi, Does this merge fix CVE-2020-13949 of C language ?

Motivation: Thrift has been released 0.14.0 with breaking changes. - apache/thrift#2208 - apache/thrift#2138 We need a workaround to share Armeria Thrift implementions from 0.9 to 0.14.0 Modifications: - Add `armeria-thrift0.14` module - Copy sources from `armeria-thrift0.13` and override `TByteBufTransport` for implementing newly added API - Catch checked exceptions which are newly added and throws unsafely for compatibility - The checked exceptions will not be raised actually - Use `armeria-thrift0.14` as the default dependencies for Armeria modules and testing Result: - You can now use Thrift 0.14.x with Armeria Thrift server and client - Fixes line#3370

Motivation: Thrift has been released 0.14.0 with breaking changes. - apache/thrift#2208 - apache/thrift#2138 We need a workaround to share Armeria Thrift implementions from 0.9 to 0.14.0 Modifications: - Add `armeria-thrift0.14` module - Copy sources from `armeria-thrift0.13` and override `TByteBufTransport` for implementing newly added API - Catch checked exceptions which are newly added and throws unsafely for compatibility - The checked exceptions will not be raised actually - Use `armeria-thrift0.14` as the default dependencies for Armeria modules and testing Result: - You can now use [libthrift 0.14.x](https://search.maven.org/artifact/org.apache.thrift/libthrift/0.14.1/jar) with Armeria Thrift server and client - Fixes #3370

dzhang-b · 2021-05-11T19:25:56Z

Hi, is this fix merged into 0.14? Could the fix backported to older version like 0.11, 0.12? Thanks

zeshuai007 force-pushed the THRIFT-5237-C_glib branch from e6f94fc to 6beeeac Compare August 1, 2020 02:38

Jens-G requested changes Aug 5, 2020

View reviewed changes

zeshuai007 force-pushed the THRIFT-5237-C_glib branch 3 times, most recently from 4f4123c to f8b2b03 Compare August 6, 2020 09:20

THRIFT-5237 Implement MAX_MESSAGE_SIZE and consolidate limits into a …

ed09218

…TConfiguration class (c_glib)

zeshuai007 force-pushed the THRIFT-5237-C_glib branch from f8b2b03 to ed09218 Compare August 6, 2020 12:03

Jens-G closed this in c80b8bb Aug 7, 2020

zeshuai007 deleted the THRIFT-5237-C_glib branch August 11, 2020 08:47

cfriedt pushed a commit to cfriedt/thrift that referenced this pull request Sep 20, 2020

THRIFT-5237 Implement MAX_MESSAGE_SIZE and consolidate limits into a …

bc0b99e

…TConfiguration class Client: c_glib Patch: Zezeng Wang This closes apache#2208

ikhoon mentioned this pull request Mar 29, 2021

Support Thrift 0.14.x line/armeria#3422

Merged

1 task

		}


		if(FALSE == ttc->checkReadBytesAvailable(THRIFT_TRANSPORT (tp->transport),

THRIFT-5237 Implement MAX_MESSAGE_SIZE and consolidate limits into a TConfiguration class (c_glib) #2208

THRIFT-5237 Implement MAX_MESSAGE_SIZE and consolidate limits into a TConfiguration class (c_glib) #2208

Uh oh!

Conversation

zeshuai007 commented Jul 29, 2020

Uh oh!

Jens-G commented Jul 29, 2020

Uh oh!

Jens-G left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeshuai007 commented Aug 6, 2020

Uh oh!

cyril867 commented Mar 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dzhang-b commented May 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cyril867 commented Mar 17, 2021 •

edited

Loading