small ssh refinements

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

small ssh refinements

Ted Unangst-6
Part one of this diff eliminates a series of u_long casts in favor of
just using native size_t types. A few other type adjustments.

Part two reorders a few conditionals in sshd for protocol 1 vs 2
handling. No functional change, just clearer and updated to reflect
protocol 2 as the default.

Index: mac.c
===================================================================
RCS file: /cvs/src/usr.bin/ssh/mac.c,v
retrieving revision 1.25
diff -u -p -r1.25 mac.c
--- mac.c 7 Nov 2013 11:58:27 -0000 1.25
+++ mac.c 21 Dec 2013 07:40:33 -0000
@@ -172,8 +172,8 @@ mac_compute(Mac *mac, u_int32_t seqno, u
  u_char b[4], nonce[8];
 
  if (mac->mac_len > sizeof(u))
- fatal("mac_compute: mac too long %u %lu",
-    mac->mac_len, (u_long)sizeof(u));
+ fatal("mac_compute: mac too long %u %zu",
+    mac->mac_len, sizeof(u));
 
  switch (mac->type) {
  case SSH_EVP:
Index: monitor_mm.c
===================================================================
RCS file: /cvs/src/usr.bin/ssh/monitor_mm.c,v
retrieving revision 1.18
diff -u -p -r1.18 monitor_mm.c
--- monitor_mm.c 8 Nov 2013 00:39:15 -0000 1.18
+++ monitor_mm.c 21 Dec 2013 07:47:30 -0000
@@ -31,6 +31,7 @@
 
 #include <errno.h>
 #include <stdarg.h>
+#include <stddef.h>
 #include <stdlib.h>
 #include <string.h>
 
@@ -42,7 +43,7 @@
 static int
 mm_compare(struct mm_share *a, struct mm_share *b)
 {
- long diff = (char *)a->address - (char *)b->address;
+ ptrdiff_t diff = (char *)a->address - (char *)b->address;
 
  if (diff == 0)
  return (0);
@@ -69,8 +70,8 @@ mm_make_entry(struct mm_master *mm, stru
 
  tmp2 = RB_INSERT(mmtree, head, tmp);
  if (tmp2 != NULL)
- fatal("mm_make_entry(%p): double address %p->%p(%lu)",
-    mm, tmp2, address, (u_long)size);
+ fatal("mm_make_entry(%p): double address %p->%p(%zu)",
+    mm, tmp2, address, size);
 
  return (tmp);
 }
@@ -96,9 +97,9 @@ mm_create(struct mm_master *mmalloc, siz
  mm->mmalloc = mmalloc;
 
  address = mmap(NULL, size, PROT_WRITE|PROT_READ, MAP_ANON|MAP_SHARED,
-    -1, (off_t)0);
+    -1, 0);
  if (address == MAP_FAILED)
- fatal("mmap(%lu): %s", (u_long)size, strerror(errno));
+ fatal("mmap(%zu): %s", size, strerror(errno));
 
  mm->address = address;
  mm->size = size;
@@ -137,7 +138,7 @@ mm_destroy(struct mm_master *mm)
  mm_freelist(mm->mmalloc, &mm->rb_allocated);
 
  if (munmap(mm->address, mm->size) == -1)
- fatal("munmap(%p, %lu): %s", mm->address, (u_long)mm->size,
+ fatal("munmap(%p, %zu): %s", mm->address, mm->size,
     strerror(errno));
  if (mm->mmalloc == NULL)
  free(mm);
@@ -152,7 +153,7 @@ mm_xmalloc(struct mm_master *mm, size_t
 
  address = mm_malloc(mm, size);
  if (address == NULL)
- fatal("%s: mm_malloc(%lu)", __func__, (u_long)size);
+ fatal("%s: mm_malloc(%zu)", __func__, size);
  memset(address, 0, size);
  return (address);
 }
@@ -187,7 +188,7 @@ mm_malloc(struct mm_master *mm, size_t s
 
  /* Does not change order in RB tree */
  mms->size -= size;
- mms->address = (u_char *)mms->address + size;
+ mms->address = (char *)mms->address + size;
 
  if (mms->size == 0) {
  RB_REMOVE(mmtree, &mm->rb_free, mms);
@@ -240,8 +241,8 @@ mm_free(struct mm_master *mm, void *addr
 
  /* Check if range does not overlap */
  if (prev != NULL && MM_ADDRESS_END(prev) > address)
- fatal("mm_free: memory corruption: %p(%lu) > %p",
-    prev->address, (u_long)prev->size, address);
+ fatal("mm_free: memory corruption: %p(%zu) > %p",
+    prev->address, prev->size, address);
 
  /* See if we can merge backwards */
  if (prev != NULL && MM_ADDRESS_END(prev) == address) {
@@ -263,8 +264,8 @@ mm_free(struct mm_master *mm, void *addr
  return;
 
  if (MM_ADDRESS_END(prev) > mms->address)
- fatal("mm_free: memory corruption: %p < %p(%lu)",
-    mms->address, prev->address, (u_long)prev->size);
+ fatal("mm_free: memory corruption: %p < %p(%zu)",
+    mms->address, prev->address, prev->size);
  if (MM_ADDRESS_END(prev) != mms->address)
  return;
 
@@ -335,12 +336,12 @@ mm_share_sync(struct mm_master **pmm, st
 void
 mm_memvalid(struct mm_master *mm, void *address, size_t size)
 {
- void *end = (u_char *)address + size;
+ void *end = (char *)address + size;
 
  if (address < mm->address)
  fatal("mm_memvalid: address too small: %p", address);
  if (end < address)
  fatal("mm_memvalid: end < address: %p < %p", end, address);
- if (end > (void *)((u_char *)mm->address + mm->size))
+ if (end > MM_ADDRESS_END(mm))
  fatal("mm_memvalid: address too large: %p", address);
 }
Index: monitor_mm.h
===================================================================
RCS file: /cvs/src/usr.bin/ssh/monitor_mm.h,v
retrieving revision 1.5
diff -u -p -r1.5 monitor_mm.h
--- monitor_mm.h 29 Apr 2008 11:20:31 -0000 1.5
+++ monitor_mm.h 21 Dec 2013 07:47:04 -0000
@@ -47,7 +47,7 @@ RB_PROTOTYPE(mmtree, mm_share, next, mm_
 
 #define MM_MINSIZE 128
 
-#define MM_ADDRESS_END(x) (void *)((u_char *)(x)->address + (x)->size)
+#define MM_ADDRESS_END(x) (void *)((char *)(x)->address + (x)->size)
 
 struct mm_master *mm_create(struct mm_master *, size_t);
 void mm_destroy(struct mm_master *);
Index: xmalloc.c
===================================================================
RCS file: /cvs/src/usr.bin/ssh/xmalloc.c,v
retrieving revision 1.28
diff -u -p -r1.28 xmalloc.c
--- xmalloc.c 17 May 2013 00:13:14 -0000 1.28
+++ xmalloc.c 21 Dec 2013 07:38:23 -0000
@@ -31,7 +31,7 @@ xmalloc(size_t size)
  fatal("xmalloc: zero size");
  ptr = malloc(size);
  if (ptr == NULL)
- fatal("xmalloc: out of memory (allocating %lu bytes)", (u_long) size);
+ fatal("xmalloc: out of memory (allocating %zu bytes)", size);
  return ptr;
 }
 
@@ -46,8 +46,8 @@ xcalloc(size_t nmemb, size_t size)
  fatal("xcalloc: nmemb * size > SIZE_T_MAX");
  ptr = calloc(nmemb, size);
  if (ptr == NULL)
- fatal("xcalloc: out of memory (allocating %lu bytes)",
-    (u_long)(size * nmemb));
+ fatal("xcalloc: out of memory (allocating %zu bytes)",
+    size * nmemb);
  return ptr;
 }
 
@@ -66,8 +66,8 @@ xrealloc(void *ptr, size_t nmemb, size_t
  else
  new_ptr = realloc(ptr, new_size);
  if (new_ptr == NULL)
- fatal("xrealloc: out of memory (new_size %lu bytes)",
-    (u_long) new_size);
+ fatal("xrealloc: out of memory (new_size %zu bytes)",
+    new_size);
  return new_ptr;
 }
 
Index: sshd.c
===================================================================
RCS file: /cvs/src/usr.bin/ssh/sshd.c,v
retrieving revision 1.412
diff -u -p -r1.412 sshd.c
--- sshd.c 6 Dec 2013 13:39:49 -0000 1.412
+++ sshd.c 21 Dec 2013 07:59:20 -0000
@@ -397,21 +397,20 @@ sshd_exchange_identification(int sock_in
  int mismatch;
  int remote_major, remote_minor;
  int major, minor;
- char *s, *newline = "\n";
+ char *s, *newline = "\r\n";
  char buf[256]; /* Must not be larger than remote_version. */
  char remote_version[256]; /* Must be at least as big as buf. */
 
- if ((options.protocol & SSH_PROTO_1) &&
-    (options.protocol & SSH_PROTO_2)) {
+ if ((options.protocol & SSH_PROTO_1)) {
  major = PROTOCOL_MAJOR_1;
- minor = 99;
- } else if (options.protocol & SSH_PROTO_2) {
+     if (options.protocol & SSH_PROTO_2)
+ minor = 99;
+ else
+ minor = PROTOCOL_MINOR_1;
+ newline = "\n";
+ } else {
  major = PROTOCOL_MAJOR_2;
  minor = PROTOCOL_MINOR_2;
- newline = "\r\n";
- } else {
- major = PROTOCOL_MAJOR_1;
- minor = PROTOCOL_MINOR_1;
  }
 
  xasprintf(&server_version_string, "SSH-%d.%d-%.100s%s%s%s",

Reply | Threaded
Open this post in threaded view
|

Re: small ssh refinements

Jeremie Courreges-Anglas-2
Ted Unangst <[hidden email]> writes:

> Part one of this diff eliminates a series of u_long casts in favor of
> just using native size_t types. A few other type adjustments.

Wouldn't that make ssh harder to port to systems where those types
aren't available?  See for example millert's last commit to
yacc/skeleton.c.

> Part two reorders a few conditionals in sshd for protocol 1 vs 2
> handling. No functional change, just clearer and updated to reflect
> protocol 2 as the default.

This part looks fine.

> Index: mac.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/ssh/mac.c,v
> retrieving revision 1.25
> diff -u -p -r1.25 mac.c
> --- mac.c 7 Nov 2013 11:58:27 -0000 1.25
> +++ mac.c 21 Dec 2013 07:40:33 -0000
> @@ -172,8 +172,8 @@ mac_compute(Mac *mac, u_int32_t seqno, u
>   u_char b[4], nonce[8];
>  
>   if (mac->mac_len > sizeof(u))
> - fatal("mac_compute: mac too long %u %lu",
> -    mac->mac_len, (u_long)sizeof(u));
> + fatal("mac_compute: mac too long %u %zu",
> +    mac->mac_len, sizeof(u));
>  
>   switch (mac->type) {
>   case SSH_EVP:
> Index: monitor_mm.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/ssh/monitor_mm.c,v
> retrieving revision 1.18
> diff -u -p -r1.18 monitor_mm.c
> --- monitor_mm.c 8 Nov 2013 00:39:15 -0000 1.18
> +++ monitor_mm.c 21 Dec 2013 07:47:30 -0000
> @@ -31,6 +31,7 @@
>  
>  #include <errno.h>
>  #include <stdarg.h>
> +#include <stddef.h>
>  #include <stdlib.h>
>  #include <string.h>
>  
> @@ -42,7 +43,7 @@
>  static int
>  mm_compare(struct mm_share *a, struct mm_share *b)
>  {
> - long diff = (char *)a->address - (char *)b->address;
> + ptrdiff_t diff = (char *)a->address - (char *)b->address;
>  
>   if (diff == 0)
>   return (0);
> @@ -69,8 +70,8 @@ mm_make_entry(struct mm_master *mm, stru
>  
>   tmp2 = RB_INSERT(mmtree, head, tmp);
>   if (tmp2 != NULL)
> - fatal("mm_make_entry(%p): double address %p->%p(%lu)",
> -    mm, tmp2, address, (u_long)size);
> + fatal("mm_make_entry(%p): double address %p->%p(%zu)",
> +    mm, tmp2, address, size);
>  
>   return (tmp);
>  }
> @@ -96,9 +97,9 @@ mm_create(struct mm_master *mmalloc, siz
>   mm->mmalloc = mmalloc;
>  
>   address = mmap(NULL, size, PROT_WRITE|PROT_READ, MAP_ANON|MAP_SHARED,
> -    -1, (off_t)0);
> +    -1, 0);
>   if (address == MAP_FAILED)
> - fatal("mmap(%lu): %s", (u_long)size, strerror(errno));
> + fatal("mmap(%zu): %s", size, strerror(errno));
>  
>   mm->address = address;
>   mm->size = size;
> @@ -137,7 +138,7 @@ mm_destroy(struct mm_master *mm)
>   mm_freelist(mm->mmalloc, &mm->rb_allocated);
>  
>   if (munmap(mm->address, mm->size) == -1)
> - fatal("munmap(%p, %lu): %s", mm->address, (u_long)mm->size,
> + fatal("munmap(%p, %zu): %s", mm->address, mm->size,
>      strerror(errno));
>   if (mm->mmalloc == NULL)
>   free(mm);
> @@ -152,7 +153,7 @@ mm_xmalloc(struct mm_master *mm, size_t
>  
>   address = mm_malloc(mm, size);
>   if (address == NULL)
> - fatal("%s: mm_malloc(%lu)", __func__, (u_long)size);
> + fatal("%s: mm_malloc(%zu)", __func__, size);
>   memset(address, 0, size);
>   return (address);
>  }
> @@ -187,7 +188,7 @@ mm_malloc(struct mm_master *mm, size_t s
>  
>   /* Does not change order in RB tree */
>   mms->size -= size;
> - mms->address = (u_char *)mms->address + size;
> + mms->address = (char *)mms->address + size;
>  
>   if (mms->size == 0) {
>   RB_REMOVE(mmtree, &mm->rb_free, mms);
> @@ -240,8 +241,8 @@ mm_free(struct mm_master *mm, void *addr
>  
>   /* Check if range does not overlap */
>   if (prev != NULL && MM_ADDRESS_END(prev) > address)
> - fatal("mm_free: memory corruption: %p(%lu) > %p",
> -    prev->address, (u_long)prev->size, address);
> + fatal("mm_free: memory corruption: %p(%zu) > %p",
> +    prev->address, prev->size, address);
>  
>   /* See if we can merge backwards */
>   if (prev != NULL && MM_ADDRESS_END(prev) == address) {
> @@ -263,8 +264,8 @@ mm_free(struct mm_master *mm, void *addr
>   return;
>  
>   if (MM_ADDRESS_END(prev) > mms->address)
> - fatal("mm_free: memory corruption: %p < %p(%lu)",
> -    mms->address, prev->address, (u_long)prev->size);
> + fatal("mm_free: memory corruption: %p < %p(%zu)",
> +    mms->address, prev->address, prev->size);
>   if (MM_ADDRESS_END(prev) != mms->address)
>   return;
>  
> @@ -335,12 +336,12 @@ mm_share_sync(struct mm_master **pmm, st
>  void
>  mm_memvalid(struct mm_master *mm, void *address, size_t size)
>  {
> - void *end = (u_char *)address + size;
> + void *end = (char *)address + size;
>  
>   if (address < mm->address)
>   fatal("mm_memvalid: address too small: %p", address);
>   if (end < address)
>   fatal("mm_memvalid: end < address: %p < %p", end, address);
> - if (end > (void *)((u_char *)mm->address + mm->size))
> + if (end > MM_ADDRESS_END(mm))
>   fatal("mm_memvalid: address too large: %p", address);
>  }
> Index: monitor_mm.h
> ===================================================================
> RCS file: /cvs/src/usr.bin/ssh/monitor_mm.h,v
> retrieving revision 1.5
> diff -u -p -r1.5 monitor_mm.h
> --- monitor_mm.h 29 Apr 2008 11:20:31 -0000 1.5
> +++ monitor_mm.h 21 Dec 2013 07:47:04 -0000
> @@ -47,7 +47,7 @@ RB_PROTOTYPE(mmtree, mm_share, next, mm_
>  
>  #define MM_MINSIZE 128
>  
> -#define MM_ADDRESS_END(x) (void *)((u_char *)(x)->address + (x)->size)
> +#define MM_ADDRESS_END(x) (void *)((char *)(x)->address + (x)->size)
>  
>  struct mm_master *mm_create(struct mm_master *, size_t);
>  void mm_destroy(struct mm_master *);
> Index: xmalloc.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/ssh/xmalloc.c,v
> retrieving revision 1.28
> diff -u -p -r1.28 xmalloc.c
> --- xmalloc.c 17 May 2013 00:13:14 -0000 1.28
> +++ xmalloc.c 21 Dec 2013 07:38:23 -0000
> @@ -31,7 +31,7 @@ xmalloc(size_t size)
>   fatal("xmalloc: zero size");
>   ptr = malloc(size);
>   if (ptr == NULL)
> - fatal("xmalloc: out of memory (allocating %lu bytes)", (u_long) size);
> + fatal("xmalloc: out of memory (allocating %zu bytes)", size);
>   return ptr;
>  }
>  
> @@ -46,8 +46,8 @@ xcalloc(size_t nmemb, size_t size)
>   fatal("xcalloc: nmemb * size > SIZE_T_MAX");
>   ptr = calloc(nmemb, size);
>   if (ptr == NULL)
> - fatal("xcalloc: out of memory (allocating %lu bytes)",
> -    (u_long)(size * nmemb));
> + fatal("xcalloc: out of memory (allocating %zu bytes)",
> +    size * nmemb);
>   return ptr;
>  }
>  
> @@ -66,8 +66,8 @@ xrealloc(void *ptr, size_t nmemb, size_t
>   else
>   new_ptr = realloc(ptr, new_size);
>   if (new_ptr == NULL)
> - fatal("xrealloc: out of memory (new_size %lu bytes)",
> -    (u_long) new_size);
> + fatal("xrealloc: out of memory (new_size %zu bytes)",
> +    new_size);
>   return new_ptr;
>  }
>  
> Index: sshd.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/ssh/sshd.c,v
> retrieving revision 1.412
> diff -u -p -r1.412 sshd.c
> --- sshd.c 6 Dec 2013 13:39:49 -0000 1.412
> +++ sshd.c 21 Dec 2013 07:59:20 -0000
> @@ -397,21 +397,20 @@ sshd_exchange_identification(int sock_in
>   int mismatch;
>   int remote_major, remote_minor;
>   int major, minor;
> - char *s, *newline = "\n";
> + char *s, *newline = "\r\n";
>   char buf[256]; /* Must not be larger than remote_version. */
>   char remote_version[256]; /* Must be at least as big as buf. */
>  
> - if ((options.protocol & SSH_PROTO_1) &&
> -    (options.protocol & SSH_PROTO_2)) {
> + if ((options.protocol & SSH_PROTO_1)) {
>   major = PROTOCOL_MAJOR_1;
> - minor = 99;
> - } else if (options.protocol & SSH_PROTO_2) {
> +     if (options.protocol & SSH_PROTO_2)
> + minor = 99;
> + else
> + minor = PROTOCOL_MINOR_1;
> + newline = "\n";
> + } else {
>   major = PROTOCOL_MAJOR_2;
>   minor = PROTOCOL_MINOR_2;
> - newline = "\r\n";
> - } else {
> - major = PROTOCOL_MAJOR_1;
> - minor = PROTOCOL_MINOR_1;
>   }
>  
>   xasprintf(&server_version_string, "SSH-%d.%d-%.100s%s%s%s",
>

--
jca | PGP: 0x06A11494 / 61DB D9A0 00A4 67CF 2A90  8961 6191 8FBF 06A1 1494

Reply | Threaded
Open this post in threaded view
|

Re: small ssh refinements

Ted Unangst-6
In reply to this post by Ted Unangst-6
On Mon, Dec 23, 2013 at 10:25, Jérémie Courrèges-Anglas wrote:
> Ted Unangst <[hidden email]> writes:
>
>> Part one of this diff eliminates a series of u_long casts in favor of
>> just using native size_t types. A few other type adjustments.
>
> Wouldn't that make ssh harder to port to systems where those types
> aren't available?  See for example millert's last commit to
> yacc/skeleton.c.

What are these systems?

The counter argument is that using long makes the code harder to port
to systems where long and ptrdiff_t are different sizes. And by harder
to port, I mean compiles fine but executes incorrectly. I consider
that a worse problem.

If ptrdiff_t is really out, I think I'd prefer to take my chances
with ssize_t being the right size. (there are 3 occurrences of
ptrdiff_t in umac.c too).


Reply | Threaded
Open this post in threaded view
|

Re: small ssh refinements

Darren Tucker
On Mon, Dec 23, 2013 at 10:21:59AM -0500, Ted Unangst wrote:
> On Mon, Dec 23, 2013 at 10:25, Jérémie Courrèges-Anglas wrote:
> > Ted Unangst <[hidden email]> writes:
> >
> >> Part one of this diff eliminates a series of u_long casts in favor of
> >> just using native size_t types. A few other type adjustments.
> >
> > Wouldn't that make ssh harder to port to systems where those types
> > aren't available?  See for example millert's last commit to
> > yacc/skeleton.c.

There's two types of porting pain: one-off and constant.

Not having a type like ptrdiff_t is the former.  It's a problem that
can be dealt with once, eg testing for it in configure and typdef'ing
as appropriate.  Not having %z is in the same boat (although a bit more
work to write a test for) since we already have a vsnprintf implementation
in the compat library and all the log messages use vsnprintf.

Constant pain is when we have to make changes to the portable code and
patches no longer apply.  These are long-term pain because they keep
slowing things down (and, if we get it wrong, cause other problems)
so should be weighed up to make sure the benefit is worth the effort.

I haven't been through the diff yet to see how much of each type is in
there :-)  I do note that %z is in SuSv3 (but not v2) so anything not
having it is likely to be old but not necessarily obsolete.

> What are these systems?
>
> The counter argument is that using long makes the code harder to port
> to systems where long and ptrdiff_t are different sizes. And by harder
> to port, I mean compiles fine but executes incorrectly. I consider
> that a worse problem.
>
> If ptrdiff_t is really out, I think I'd prefer to take my chances
> with ssize_t being the right size. (there are 3 occurrences of
> ptrdiff_t in umac.c too).

Given that it's already in use and we haven't had any complaints that
I'm aware of I have no objection to adding more ptrdiff_t.  I'll look
through the rest of the diff shortly.

--
Darren Tucker (dtucker at zip.com.au)
GPG key 8FF4FA69 / D9A3 86E9 7EEE AF4B B2D4  37C9 C982 80C7 8FF4 FA69
    Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.

Reply | Threaded
Open this post in threaded view
|

Re: small ssh refinements

Todd C. Miller
In reply to this post by Ted Unangst-6
Note that yacc skeleton is a special case since there is no configure
goo to rely on.  With portable ssh we have configure checks available.

 - todd