Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug fixes in c interface #118

Merged
merged 7 commits into from
May 28, 2023
Merged

bug fixes in c interface #118

merged 7 commits into from
May 28, 2023

Conversation

craff
Copy link
Contributor

@craff craff commented May 24, 2023

While working on another PR I spotted 3 bugs (and one security issue)

  • access outside of the buffer is possible in Ssl.read and Ssl.write: start sign is not tested
  • ERR_clear_error() is missing in 4 places while it is required in the spec
  • a missing "_blocking" in my last PR

I don't think 3 separate PR are needed, as it is a small PR, but I can do it if you wish.

@craff
Copy link
Contributor Author

craff commented May 24, 2023

Also spotted that my implementation for checking if flush must be retried is not correct. One must use BIO_should_retry().

src/ssl_stubs.c Outdated
@@ -1828,7 +1840,7 @@ CAMLprim value ocaml_ssl_flush(value socket)
if (ret != 1) {
caml_acquire_runtime_system();
caml_raise_with_arg(*caml_named_value("ssl_exn_flush_error"),
Val_bool(ret==-1));
Val_bool(BIO_should_retry(bio)));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can these be a separate PR? I think I have a different interpretation of the docs, so let's discuss there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I found the doc not really clear on that point (and a few other too)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, this is PR #120.

Copy link
Collaborator

@anmonteiro anmonteiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think other functions that call ERR_get_error are still missing ERR_clear_error calls, e.g. ocaml_ssl_ctx_add_extra_chain_cert.

EDIT: I'm wrong, ERR_get_error clears the error queue, ERR_clear_error is only needed before calls to SSL_get_error.

if (Int_val(start) + Int_val(length) > caml_string_length(buffer))
caml_invalid_argument("Buffer too short.");
caml_invalid_argument("Ssl.write: Buffer too short.");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still wonder if this should refer to the actual function name (write vs write_blocking) so that the caller knows the real source. Perhaps not that important.

@anmonteiro anmonteiro merged commit 9f8c31a into savonet:master May 28, 2023
@craff
Copy link
Contributor Author

craff commented May 31, 2023 via email

anmonteiro added a commit to anmonteiro/opam-repository that referenced this pull request Jun 2, 2023
CHANGES:

- Raise an error when `Ssl.flush` isn't successful (savonet/ocaml-ssl#104, savonet/ocaml-ssl#120)
- Add an API-compatible `Ssl.Runtime_lock` module. The functions in this module
  don't release the OCaml runtime lock. While they don't allow other OCaml
  threads to run concurrently, they don't perform any copying in the underlying
  data, leading certain workloads to be faster than their counterparts that
  release the lock. (savonet/ocaml-ssl#106)
- Guarantee `Ssl.output_string` writes the whole string by retrying the
  operation with unwritten bytes (savonet/ocaml-ssl#103, savonet/ocaml-ssl#116)
- Fix calls in C stubs that need to call `ERR_clear_error` before the underlying
  OpenSSL call (savonet/ocaml-ssl#118)
- Add a module `Ssl.Error` to retrieve OpenSSL errors in a structured way (savonet/ocaml-ssl#119)
- Deprecate Ssl.{SSLv23,SSLv3,TLSv1,TLSv1_1}, which were were formally
  deprecated in March 2021 and earlier (savonet/ocaml-ssl#115).
anmonteiro pushed a commit to craff/ocaml-ssl that referenced this pull request Jun 2, 2023
bug fixes in c interface

wrong way for checking if we need retry in Ssl.flush

missplaced SSL_clear_error for shutdown and finish all Runtime_lock functions

Added Error_want_retry_verify in ssl.ml/mli

Added a safer conversion to type Ssl.ssl_error

remove all raw_xxx functions and use shadowing

Simplify error treatment

wrong function name in error.

added a comment to explain -2 in flush

Use BIO_should_retry in flush (savonet#120)

---------

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

fix issue savonet#103 for output_string, output_char and input_int (savonet#116)

* fix issue savonet#103 for output_string output_char and input_int

* Update CHANGES.md

* Update CHANGES.md

* Apply suggestions from code review

---------

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

bug fixes in c interface (savonet#118)

* bug fixes in c interface

* wrong way for checking if we need retry in Ssl.flush

* missplaced SSL_clear_error for whutdown

* revert one change to put in a separate PR.

* forgot one case

* Update CHANGES.md

---------

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

added comment not to forget to update macro in ssl_stubs.c when adding errors

revert error treatment
anmonteiro pushed a commit to craff/ocaml-ssl that referenced this pull request Jun 2, 2023
bug fixes in c interface

wrong way for checking if we need retry in Ssl.flush

missplaced SSL_clear_error for shutdown and finish all Runtime_lock functions

Added Error_want_retry_verify in ssl.ml/mli

Added a safer conversion to type Ssl.ssl_error

remove all raw_xxx functions and use shadowing

Simplify error treatment

wrong function name in error.

added a comment to explain -2 in flush

Use BIO_should_retry in flush (savonet#120)

---------

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

fix issue savonet#103 for output_string, output_char and input_int (savonet#116)

* fix issue savonet#103 for output_string output_char and input_int

* Update CHANGES.md

* Update CHANGES.md

* Apply suggestions from code review

---------

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

bug fixes in c interface (savonet#118)

* bug fixes in c interface

* wrong way for checking if we need retry in Ssl.flush

* missplaced SSL_clear_error for whutdown

* revert one change to put in a separate PR.

* forgot one case

* Update CHANGES.md

---------

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

added comment not to forget to update macro in ssl_stubs.c when adding errors

revert error treatment
anmonteiro pushed a commit to craff/ocaml-ssl that referenced this pull request Jun 2, 2023
bug fixes in c interface

wrong way for checking if we need retry in Ssl.flush

missplaced SSL_clear_error for shutdown and finish all Runtime_lock functions

Added Error_want_retry_verify in ssl.ml/mli

Added a safer conversion to type Ssl.ssl_error

remove all raw_xxx functions and use shadowing

Simplify error treatment

wrong function name in error.

added a comment to explain -2 in flush

Use BIO_should_retry in flush (savonet#120)

---------

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

fix issue savonet#103 for output_string, output_char and input_int (savonet#116)

* fix issue savonet#103 for output_string output_char and input_int

* Update CHANGES.md

* Update CHANGES.md

* Apply suggestions from code review

---------

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

bug fixes in c interface (savonet#118)

* bug fixes in c interface

* wrong way for checking if we need retry in Ssl.flush

* missplaced SSL_clear_error for whutdown

* revert one change to put in a separate PR.

* forgot one case

* Update CHANGES.md

---------

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

added comment not to forget to update macro in ssl_stubs.c when adding errors

revert error treatment
anmonteiro pushed a commit that referenced this pull request Jun 2, 2023
bug fixes in c interface

wrong way for checking if we need retry in Ssl.flush

missplaced SSL_clear_error for shutdown and finish all Runtime_lock functions

Added Error_want_retry_verify in ssl.ml/mli

Added a safer conversion to type Ssl.ssl_error

remove all raw_xxx functions and use shadowing

Simplify error treatment

wrong function name in error.

added a comment to explain -2 in flush

Use BIO_should_retry in flush (#120)

---------

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

fix issue #103 for output_string, output_char and input_int (#116)

* fix issue #103 for output_string output_char and input_int

* Update CHANGES.md

* Update CHANGES.md

* Apply suggestions from code review

---------

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

bug fixes in c interface (#118)

* bug fixes in c interface

* wrong way for checking if we need retry in Ssl.flush

* missplaced SSL_clear_error for whutdown

* revert one change to put in a separate PR.

* forgot one case

* Update CHANGES.md

---------

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

added comment not to forget to update macro in ssl_stubs.c when adding errors

revert error treatment
anmonteiro added a commit to anmonteiro/opam-repository that referenced this pull request Jun 3, 2023
CHANGES:

- Raise an error when `Ssl.flush` isn't successful (savonet/ocaml-ssl#104, savonet/ocaml-ssl#120)
- Add an API-compatible `Ssl.Runtime_lock` module. The functions in this module
  don't release the OCaml runtime lock. While they don't allow other OCaml
  threads to run concurrently, they don't perform any copying in the underlying
  data, leading certain workloads to be faster than their counterparts that
  release the lock. (savonet/ocaml-ssl#106)
- Guarantee `Ssl.output_string` writes the whole string by retrying the
  operation with unwritten bytes (savonet/ocaml-ssl#103, savonet/ocaml-ssl#116)
- Fix calls in C stubs that need to call `ERR_clear_error` before the underlying
  OpenSSL call (savonet/ocaml-ssl#118)
- Add a module `Ssl.Error` to retrieve OpenSSL errors in a structured way (savonet/ocaml-ssl#119)
- Deprecate Ssl.{SSLv23,SSLv3,TLSv1,TLSv1_1}, which were were formally
  deprecated in March 2021 and earlier (savonet/ocaml-ssl#115).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants