thr3ads.net - Libguestfs - [Libguestfs] [libnbd PATCH] states: Avoid magic number for h->tls [Sep 2019]

If this information is useful, please help other people find it:
Share via:

Eric Blake

2019-Sep-16 19:29 UTC

[Libguestfs] [libnbd PATCH] states: Avoid magic number for h->tls

When we moved to an enum instead of raw int for nbd_set_tls(), we
should have also updated our code to prefer the enum values.  While at
it, improve the grammar of error messages (confusing since 632196ec,
and copy-and-pasted into more locations since then).

Fixes: 4488cf2a
Thanks: Rich Jones
---

Rich noticed this while reviewing the patch for today's CVE fix.  It's
not a show-stopper if this doesn't get included in today's releases.

 generator/states-newstyle-opt-starttls.c | 8 ++++----
 generator/states-newstyle.c              | 4 ++--
 generator/states-oldstyle.c              | 6 +++---
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/generator/states-newstyle-opt-starttls.c
b/generator/states-newstyle-opt-starttls.c
index 0a18db0..b050ce0 100644
--- a/generator/states-newstyle-opt-starttls.c
+++ b/generator/states-newstyle-opt-starttls.c
@@ -21,7 +21,7 @@
 /* STATE MACHINE */ {
  NEWSTYLE.OPT_STARTTLS.START:
   /* If TLS was not requested we skip this option and go to the next one. */
-  if (!h->tls) {
+  if (h->tls == LIBNBD_TLS_DISABLE) {
     SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START);
     return 0;
   }
@@ -88,13 +88,13 @@
       return 0;
     }

-    /* Server refused to upgrade to TLS.  If h->tls is not require (2)
+    /* Server refused to upgrade to TLS.  If h->tls is not 'require'
(2)
      * then we can continue unencrypted.
      */
-    if (h->tls == 2) {
+    if (h->tls == LIBNBD_TLS_REQUIRE) {
       SET_NEXT_STATE (%.DEAD);
       set_error (ENOTSUP, "handshake: server refused TLS, "
-                 "but handle TLS setting is require (2)");
+                 "but handle TLS setting is 'require' (2)");
       return 0;
     }

diff --git a/generator/states-newstyle.c b/generator/states-newstyle.c
index c8f817e..b4f2b80 100644
--- a/generator/states-newstyle.c
+++ b/generator/states-newstyle.c
@@ -129,10 +129,10 @@ handle_reply_error (struct nbd_handle *h)

   h->gflags = be16toh (h->gflags);
   if ((h->gflags & NBD_FLAG_FIXED_NEWSTYLE) == 0 &&
-      h->tls == 2) {
+      h->tls == LIBNBD_TLS_REQUIRE) {
     SET_NEXT_STATE (%.DEAD);
     set_error (ENOTSUP, "handshake: server is not fixed newstyle, "
-               "but handle TLS setting is require (2)");
+               "but handle TLS setting is 'require' (2)");
     return 0;
   }

diff --git a/generator/states-oldstyle.c b/generator/states-oldstyle.c
index 1aff185..babefc0 100644
--- a/generator/states-oldstyle.c
+++ b/generator/states-oldstyle.c
@@ -46,13 +46,13 @@
   gflags = be16toh (h->sbuf.old_handshake.gflags);
   eflags = be16toh (h->sbuf.old_handshake.eflags);

-  /* Server is unable to upgrade to TLS.  If h->tls is not require (2)
+  /* Server is unable to upgrade to TLS.  If h->tls is not 'require'
(2)
    * then we can continue unencrypted.
    */
-  if (h->tls == 2) {
+  if (h->tls == LIBNBD_TLS_REQUIRE) {
     SET_NEXT_STATE (%.DEAD);
     set_error (ENOTSUP, "handshake: server is oldstyle, "
-               "but handle TLS setting is require (2)");
+               "but handle TLS setting is 'require' (2)");
     return 0;
   }

-- 
2.21.0

Richard W.M. Jones

2019-Sep-17 08:29 UTC

head link

Re: [Libguestfs] [libnbd PATCH] states: Avoid magic number for h->tls

On Mon, Sep 16, 2019 at 02:29:38PM -0500, Eric Blake
wrote:> When we moved to an enum instead of raw int for nbd_set_tls(), we
> should have also updated our code to prefer the enum values.  While at
> it, improve the grammar of error messages (confusing since 632196ec,
> and copy-and-pasted into more locations since then).
> 
> Fixes: 4488cf2a
> Thanks: Rich Jones
> ---
> 
> Rich noticed this while reviewing the patch for today's CVE fix. 
It's
> not a show-stopper if this doesn't get included in today's
releases.
> 
>  generator/states-newstyle-opt-starttls.c | 8 ++++----
>  generator/states-newstyle.c              | 4 ++--
>  generator/states-oldstyle.c              | 6 +++---
>  3 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/generator/states-newstyle-opt-starttls.c
b/generator/states-newstyle-opt-starttls.c
> index 0a18db0..b050ce0 100644
> --- a/generator/states-newstyle-opt-starttls.c
> +++ b/generator/states-newstyle-opt-starttls.c
> @@ -21,7 +21,7 @@
>  /* STATE MACHINE */ {
>   NEWSTYLE.OPT_STARTTLS.START:
>    /* If TLS was not requested we skip this option and go to the next one.
*/
> -  if (!h->tls) {
> +  if (h->tls == LIBNBD_TLS_DISABLE) {
>      SET_NEXT_STATE (%^OPT_STRUCTURED_REPLY.START);
>      return 0;
>    }
> @@ -88,13 +88,13 @@
>        return 0;
>      }
> 
> -    /* Server refused to upgrade to TLS.  If h->tls is not require (2)
> +    /* Server refused to upgrade to TLS.  If h->tls is not
'require' (2)
>       * then we can continue unencrypted.
>       */
> -    if (h->tls == 2) {
> +    if (h->tls == LIBNBD_TLS_REQUIRE) {
>        SET_NEXT_STATE (%.DEAD);
>        set_error (ENOTSUP, "handshake: server refused TLS, "
> -                 "but handle TLS setting is require (2)");
> +                 "but handle TLS setting is 'require'
(2)");
>        return 0;
>      }
> 
> diff --git a/generator/states-newstyle.c b/generator/states-newstyle.c
> index c8f817e..b4f2b80 100644
> --- a/generator/states-newstyle.c
> +++ b/generator/states-newstyle.c
> @@ -129,10 +129,10 @@ handle_reply_error (struct nbd_handle *h)
> 
>    h->gflags = be16toh (h->gflags);
>    if ((h->gflags & NBD_FLAG_FIXED_NEWSTYLE) == 0 &&
> -      h->tls == 2) {
> +      h->tls == LIBNBD_TLS_REQUIRE) {
>      SET_NEXT_STATE (%.DEAD);
>      set_error (ENOTSUP, "handshake: server is not fixed newstyle,
"
> -               "but handle TLS setting is require (2)");
> +               "but handle TLS setting is 'require'
(2)");
>      return 0;
>    }
> 
> diff --git a/generator/states-oldstyle.c b/generator/states-oldstyle.c
> index 1aff185..babefc0 100644
> --- a/generator/states-oldstyle.c
> +++ b/generator/states-oldstyle.c
> @@ -46,13 +46,13 @@
>    gflags = be16toh (h->sbuf.old_handshake.gflags);
>    eflags = be16toh (h->sbuf.old_handshake.eflags);
> 
> -  /* Server is unable to upgrade to TLS.  If h->tls is not require (2)
> +  /* Server is unable to upgrade to TLS.  If h->tls is not
'require' (2)
>     * then we can continue unencrypted.
>     */
> -  if (h->tls == 2) {
> +  if (h->tls == LIBNBD_TLS_REQUIRE) {
>      SET_NEXT_STATE (%.DEAD);
>      set_error (ENOTSUP, "handshake: server is oldstyle, "
> -               "but handle TLS setting is require (2)");
> +               "but handle TLS setting is 'require'
(2)");
>      return 0;
>    }
> 
ACK

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

Possibly Parallel Threads

Search for more seemingly similar threads

Libguestfs - Sep 2019 - [libnbd PATCH] states: Avoid magic number for h->tls

[Libguestfs] [libnbd PATCH] states: Avoid magic number for h->tls

Re: [Libguestfs] [libnbd PATCH] states: Avoid magic number for h->tls

Possibly Parallel Threads