Add a 'max_send_frag' option to configure maximum size of send fragments #3141

FdaSilvaYY · 2017-04-06T22:15:12Z

Checklist

documentation is added or updated
tests are added or updated

Noticed while working on #1008

richsalz

minor nits.

richsalz · 2017-04-07T00:56:06Z

apps/s_client.c

@@ -1432,6 +1442,11 @@ int s_client_main(int argc, char **argv)
        goto end;
    }

+    if (max_send_fragment > SSL3_RT_MAX_PLAIN_LENGTH) {
+        BIO_printf(bio_err, "Bad max send fragment size\n");


plug put "%s: " and print prog.

richsalz · 2017-04-07T00:56:34Z

apps/s_server.c

@@ -1543,6 +1556,11 @@ int s_server_main(int argc, char *argv[])
    }
 #endif

+    if (max_send_fragment > SSL3_RT_MAX_PLAIN_LENGTH) {
+        BIO_printf(bio_err, "Bad max send fragment size\n");


output prog name here, too. I know it's not always done, but let's get in the habit of fixing these.

I will fix some more in another PR ;)

richsalz · 2017-04-07T00:57:08Z

doc/man1/s_client.pod

+=item B<-max_send_frag int>
+
+The maximum size of data fragment to send.  See
+L<SSL_CTX_set_max_send_fragment(3)> for further information.


If there's a SEE ALSO section, add a link there, too.

dot-asm · 2017-04-07T09:32:15Z

doc/man1/s_client.pod

+=item B<-max_send_frag int>
+
+The maximum size of data fragment to send.
+See L<SSL_CTX_set_max_send_fragment(3)> for further information.


Well, SSL_CTX_set_max_send_fragment says that it "will only accept a value in the range 512 - SSL3_RT_MAX_PLAIN_LENGTH." This kind of suggests that one should check for it's return value and at least issue warning that attempt is failed.

I mean as it stands now it only checks for upper limit.

Checks added !

levitte · 2017-04-07T09:49:55Z

apps/s_server.c

@@ -1741,6 +1759,10 @@ int s_server_main(int argc, char *argv[])
    if (async) {
        SSL_CTX_set_mode(ctx, SSL_MODE_ASYNC);
    }
+
+    if (max_send_fragment > 0)
+        SSL_CTX_set_max_send_fragment(ctx, max_send_fragment);


The returned value should be checked. 0 indicates failure... and really, that should also be done for the three calls below as well...

levitte · 2017-04-12T17:22:19Z

apps/s_client.c

        goto end;
    }

    if (max_pipelines > SSL_MAX_PIPELINES) {
-        BIO_printf(bio_err, "Bad max pipelines value\n");
+        BIO_printf(bio_err, "%s: too large max-pipelines value\n", prog);
        goto end;
    }


Ok, all these checks are a bit over the top. What happens when the libssl code changes those ranges, will we have to keep track of that and update the values here as well? It should be enough that SSL_CTX_set_max_send_fragment(), SSL_CTX_set_split_send_fragment(), SSL_CTX_set_max_pipelines() and SSL_ friends return 0 when the given argument is out of range...

levitte · 2017-04-12T17:25:01Z

apps/s_client.c

+
+    if (max_send_fragment > 0
+        && !SSL_CTX_set_max_send_fragment(ctx, max_send_fragment)) {
+        BIO_printf(bio_err, "%s: Error setting max send fragment size\n", prog);


I suggest this:

if (!SSL_CTX_set_max_send_fragment(ctx, max_send_fragment)) { BIO_printf(bio_err, "%s: Max send fragment size %u is out of permitted range\n", prog, max_send_fragment);

levitte · 2017-04-12T17:25:41Z

apps/s_client.c

+
+    if (split_send_fragment > 0
+        && !SSL_CTX_set_split_send_fragment(ctx, split_send_fragment)) {
+        BIO_printf(bio_err, "%s: Error setting split send fragment size\n", prog);


I suggest this:

if (!SSL_CTX_set_split_send_fragment(ctx, split_send_fragment)) { BIO_printf(bio_err, "%s: Split send fragment size %u is out of permitted range\n", prog, split_send_fragment);

levitte · 2017-04-12T17:26:31Z

apps/s_client.c

+
+    if (max_pipelines > 0
+        && !SSL_CTX_set_max_pipelines(ctx, max_pipelines)) {
+        BIO_printf(bio_err, "%s: Error setting max pipelines\n", prog);


I suggest this:

if (!SSL_CTX_set_max_pipelines(ctx, max_pipelines)) { BIO_printf(bio_err, "%s: Max pipelines %u is out of permitted range\n", prog, max_pipelines);

levitte · 2017-04-12T17:27:00Z

apps/s_server.c

    if (max_pipelines > SSL_MAX_PIPELINES) {
-        BIO_printf(bio_err, "Bad max pipelines value\n");
+        BIO_printf(bio_err, "%s:too large max-pipelines value\n", prog);
        goto end;
    }


Same comment here as for s_client.c

levitte · 2017-04-12T17:27:28Z

apps/s_server.c

+    if (max_pipelines > 0
+        && !SSL_CTX_set_max_pipelines(ctx, max_pipelines)) {
+        BIO_printf(bio_err, "%s: Error setting max pipelines value\n", prog);
+        goto end;


Same suggestions here as in s_client.c

FdaSilvaYY · 2017-04-12T21:10:24Z

@levitte : rebased and error message updated.

levitte · 2017-04-12T21:26:21Z

I think you misunderstood me. It wasn't just about the message, but also about all that unnecessary checking of values, which is done in SSL_CTX_set_max_send_fragment() et al anyway!

FdaSilvaYY · 2017-04-26T21:38:10Z

@levitte : rebased on top of last master. It it OK ?

levitte · 2017-04-26T21:45:01Z

apps/s_client.c

@@ -1444,6 +1440,7 @@ int s_client_main(int argc, char **argv)
        goto end;
    }

+<<<<<<< HEAD


I think this line should go ;-)

Remove hardcoded bound checkings.

FdaSilvaYY · 2017-04-27T20:25:49Z

Rebased and conflict fixed. Ready to merge .

Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #3141)

Remove hardcoded bound checkings. Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #3141)

levitte · 2017-04-28T13:51:15Z

Merged. 28e5ea8 to 36b2cfb

richsalz · 2017-04-28T13:59:13Z

should the doc say that the RELEASE-BUFFERS flag is needed for this to actually reduce memory?

Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#3141) cherry-pick from 28e5ea8

Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#3141) Cherry-oick from 6788785

Remove hardcoded bound checkings. Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#3141) cherry-pick from 36b2cfb

Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#3141) cherry-pick from 28e5ea8

Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#3141) Cherry-oick from 6788785

Remove hardcoded bound checkings. Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from openssl#3141) cherry-pick from 36b2cfb

FdaSilvaYY changed the title ~~Add a 'max_send_frag' option to configure maximun size of send fragments~~ Add a 'max_send_frag' option to configure maximum size of send fragments Apr 6, 2017

FdaSilvaYY force-pushed the add-max_send_fragment-to-apps branch from c058a9e to 88934fb Compare April 6, 2017 22:16

richsalz approved these changes Apr 7, 2017

View reviewed changes

FdaSilvaYY force-pushed the add-max_send_fragment-to-apps branch from 88934fb to 5fb6f61 Compare April 7, 2017 07:02

dot-asm reviewed Apr 7, 2017

View reviewed changes

levitte requested changes Apr 7, 2017

View reviewed changes

FdaSilvaYY force-pushed the add-max_send_fragment-to-apps branch 6 times, most recently from dad7fe6 to 7d81e34 Compare April 7, 2017 18:27

dot-asm approved these changes Apr 7, 2017

View reviewed changes

dot-asm added branch: master Merge to master branch approval: done This pull request has the required number of approvals labels Apr 7, 2017

FdaSilvaYY force-pushed the add-max_send_fragment-to-apps branch from 7d81e34 to ac71e51 Compare April 10, 2017 19:35

levitte requested changes Apr 12, 2017

View reviewed changes

levitte removed the approval: done This pull request has the required number of approvals label Apr 12, 2017

FdaSilvaYY force-pushed the add-max_send_fragment-to-apps branch from ac71e51 to 645a971 Compare April 12, 2017 20:35

FdaSilvaYY force-pushed the add-max_send_fragment-to-apps branch from 645a971 to 00d2b4d Compare April 14, 2017 21:51

FdaSilvaYY force-pushed the add-max_send_fragment-to-apps branch from 00d2b4d to ec3798d Compare April 26, 2017 21:36

levitte requested changes Apr 26, 2017

View reviewed changes

FdaSilvaYY force-pushed the add-max_send_fragment-to-apps branch from ec3798d to 1c44fc7 Compare April 26, 2017 22:18

levitte approved these changes Apr 27, 2017

View reviewed changes

levitte added the approval: done This pull request has the required number of approvals label Apr 27, 2017

FdaSilvaYY added 2 commits April 27, 2017 22:24

Add a 'max_send_frag' option to configure maximum size of send fragments

9ce3da3

Output prog name within error message

c9daaa3

Add checks on return code when applying some settings.

b9593a3

Remove hardcoded bound checkings.

FdaSilvaYY force-pushed the add-max_send_fragment-to-apps branch from 1c44fc7 to b9593a3 Compare April 27, 2017 20:24

levitte pushed a commit that referenced this pull request Apr 28, 2017

Output prog name within error message

6788785

Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #3141)

levitte closed this Apr 28, 2017

FdaSilvaYY deleted the add-max_send_fragment-to-apps branch May 2, 2017 20:17

This was referenced May 4, 2017

[1.1.0] Add a 'max_send_frag' option to configure maximum size of send fragments #3390

Closed

[1.1.0] Add a 'max_send_frag' option to configure maximum size of send fragments #3391

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a 'max_send_frag' option to configure maximum size of send fragments #3141

Add a 'max_send_frag' option to configure maximum size of send fragments #3141

FdaSilvaYY commented Apr 6, 2017 •

edited

richsalz left a comment

richsalz Apr 7, 2017

richsalz Apr 7, 2017

FdaSilvaYY Apr 7, 2017 •

edited

richsalz Apr 7, 2017

dot-asm Apr 7, 2017

dot-asm Apr 7, 2017

FdaSilvaYY Apr 7, 2017

levitte Apr 7, 2017

levitte Apr 12, 2017

levitte Apr 12, 2017

levitte Apr 12, 2017

levitte Apr 12, 2017

levitte Apr 12, 2017

levitte Apr 12, 2017

FdaSilvaYY commented Apr 12, 2017

levitte commented Apr 12, 2017

FdaSilvaYY commented Apr 26, 2017

levitte Apr 26, 2017

FdaSilvaYY Apr 26, 2017

FdaSilvaYY commented Apr 27, 2017

levitte commented Apr 28, 2017

richsalz commented Apr 28, 2017

Add a 'max_send_frag' option to configure maximum size of send fragments #3141

Add a 'max_send_frag' option to configure maximum size of send fragments #3141

Conversation

FdaSilvaYY commented Apr 6, 2017 • edited

Checklist

richsalz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FdaSilvaYY Apr 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FdaSilvaYY commented Apr 12, 2017

levitte commented Apr 12, 2017

FdaSilvaYY commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FdaSilvaYY commented Apr 27, 2017

levitte commented Apr 28, 2017

richsalz commented Apr 28, 2017

FdaSilvaYY commented Apr 6, 2017 •

edited

FdaSilvaYY Apr 7, 2017 •

edited