Re: gnumach 1.7 issue

[James Clarke]

> On 4 Jun 2016, at 15:04, James Clarke  wrote:
> 
>> On 3 Jun 2016, at 19:36, Samuel Thibault  wrote:
>> 
>> Samuel Thibault, on Fri 03 Jun 2016 20:35:13 +0200, wrote:
>>> Adam Richards, on Fri 03 Jun 2016 19:30:40 +0100, wrote:
 On rebooting and reverting to  gnumach-1.6-486.gz this is not an issue.
>>> 
>>> It could be worth bisecting the issue on the upstream git repository.
>> 
>> (there are really not much difference between the last 1.6 snapshot
>> 2:1.6+git20160502-1 and the 1.7 snapshot 2:1.7+git20160522-1)
> 
> 1.7 seems to break running Xorg for me (running latest MATE). I can’t
> move the mouse, opening htop via ssh hangs at a black screen, and then
> subsequent ssh attempts can’t connect, so I have to perform a hard
> reboot. If I don’t send *any* mouse events, htop loads, but locks up
> as soon as I move the mouse. MATE’s clock does continue to update every
> second, so some things still work. Booting 1.6+git20160502 instead works
> fine.
> 
> Looking at the changes[1], some variables types have been changed (many
> int -> unsigned, and a few int -> dev_t in xen/console.{c,h}), but I
> doubt that’s it. My guess is that it’s one of:
> 
> * b4d07d3 Fix pageout deadlock
> * 1db202e vm_map: back allocations with a red-black tree
> 
> I shall try building a kernel with 1db202e reverted.

Reverting 1db202e indeed fixes it. I also rebuilt the same source tree with
the patch commented out in d/p/series just to be sure there wasn’t a
toolchain issue, and that build *doesn’t* work, so 1db202e is either buggy
or exposing other bugs.

James



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: RFC: Runtime checking of port handling

[Samuel Thibault]

Hello,

Justus Winter, on Sat 04 Jun 2016 15:45:15 +0200, wrote:
> tl;dr: Compiler-assisted runtime checking of port handling in
> variables with automatic storage duration.  Do we want to go there?

That's interesting, but the proposed way would need annotating the whole
source code, I'm afraid we'll make mistakes there.  Passing a port to a
function that will deallocate it will probably need another macro BTW.

Ideally we'd have a static analysis tool which knows about the mig rules
for port references. Perhaps LeakSanitizer could be taught that? (we
probably want to port it to Hurd, anyway, and it seems very portable)

Samuel

Re: [PATCH hurd 1/2] xxx print fail

[Samuel Thibault]

That looks useful, yes, please commit :)

Justus Winter, on Sat 04 Jun 2016 15:47:51 +0200, wrote:
> ---
>  libshouldbeinlibc/assert-backtrace.c | 8 
>  libshouldbeinlibc/assert-backtrace.h | 6 ++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/libshouldbeinlibc/assert-backtrace.c 
> b/libshouldbeinlibc/assert-backtrace.c
> index ca23c8d..72a49e9 100644
> --- a/libshouldbeinlibc/assert-backtrace.c
> +++ b/libshouldbeinlibc/assert-backtrace.c
> @@ -76,4 +76,12 @@ __assert_perror_fail_backtrace (int errnum,
>  
>  }
>  
> +void
> +__print_fail_backtrace (const char *message, const char *file,
> +unsigned int line, const char *function)
> +{
> +  __assert_fail_base_backtrace ("%s: %s:%u: %s: %s.\n",
> + message, file, line, function);
> +}
> +
>  #endif /* ! defined NDEBUG */
> diff --git a/libshouldbeinlibc/assert-backtrace.h 
> b/libshouldbeinlibc/assert-backtrace.h
> index c54b810..b36e5b2 100644
> --- a/libshouldbeinlibc/assert-backtrace.h
> +++ b/libshouldbeinlibc/assert-backtrace.h
> @@ -42,6 +42,12 @@ void __assert_perror_fail_backtrace (int errnum,
>const char *function)
>__attribute__ ((noreturn, unused));
>  
> +/* Likewise, but prints the given MESSAGE.  */
> +void
> +__print_fail_backtrace (const char *message, const char *file,
> +unsigned int line, const char *function)
> +  __attribute__ ((noreturn, unused));
> +
>  #define assert_backtrace(expr)   
> \
>((expr)\
> ? (void) 0
> \
> -- 
> 2.1.4
> 
> 

-- 
Samuel
Linux, c'est simple : ça s'adresse à une machine qui est parfois un peu
maraboutée mais qui d'habitude n'a pas d'états d'âme. Sur Usenet y'a
plein d'humains et de primates, et ça devient vraiment gore par moment.
-+- TP in : Guide du linuxien pervers - "Le linuxien a-t-il une âme ?" -+-

[PATCH hurd 2/2] libshouldbeinlibc: add safe port handling macros

[Justus Winter]

* libshouldbeinlibc/Makefile (SRCS, installhdrs): Add new file.
* libshouldbeinlibc/machx.h: New file.
---
 libshouldbeinlibc/Makefile |  2 ++
 libshouldbeinlibc/machx.h  | 73 ++
 trans/crash.c  | 29 +-
 3 files changed, 89 insertions(+), 15 deletions(-)
 create mode 100644 libshouldbeinlibc/machx.h

diff --git a/libshouldbeinlibc/Makefile b/libshouldbeinlibc/Makefile
index 04c085b..76076f2 100644
--- a/libshouldbeinlibc/Makefile
+++ b/libshouldbeinlibc/Makefile
@@ -30,11 +30,13 @@ SRCS = termsize.c timefmt.c exec-reauth.c maptime-funcs.c \
ugids-verify-auth.c nullauth.c \
refcount.c \
assert-backtrace.c \
+   machx.h \
 
 installhdrs = idvec.h timefmt.h maptime.h \
  wire.h portinfo.h portxlate.h cacheq.h ugids.h nullauth.h \
  refcount.h \
  assert-backtrace.h \
+ machx.h \
 
 installhdrsubdir = .
 
diff --git a/libshouldbeinlibc/machx.h b/libshouldbeinlibc/machx.h
new file mode 100644
index 000..6e727de
--- /dev/null
+++ b/libshouldbeinlibc/machx.h
@@ -0,0 +1,73 @@
+/* Safe right handling routines.
+
+   Copyright (C) 2016 Free Software Foundation, Inc.
+
+   This file is part of the GNU Hurd.
+
+   The GNU Hurd is free software; you can redistribute it and/or
+   modify it under the terms of the GNU General Public License as
+   published by the Free Software Foundation; either version 2, or (at
+   your option) any later version.
+
+   The GNU Hurd is distributed in the hope that it will be useful, but
+   WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+   General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with the GNU Hurd.  If not, see .  */
+
+#ifndef __MACHX__
+#define __MACHX__
+
+#include 
+#include 
+#include 
+
+#define Mach_port_deallocate(TASK, NAME)\
+  ({\
+if (MACH_PORT_VALID (NAME)) \
+  { \
+error_t _Xerr = mach_port_deallocate ((TASK), (NAME));  \
+if (_Xerr)  \
+  { \
+char _Xmessage[1024];   \
+snprintf (_Xmessage, sizeof _Xmessage,  \
+  "error deallocating "#NAME": %s", strerror (_Xerr)); \
+__print_fail_backtrace (_Xmessage, __FILE__, __LINE__,  \
+__PRETTY_FUNCTION__);   \
+  } \
+NAME = MACH_PORT_NULL;  \
+  } \
+  })
+
+#define Mach_port_move(NAME)\
+  ({\
+mach_port_t _Xport = (NAME);\
+NAME = MACH_PORT_NULL;  \
+_Xport; \
+  })
+
+#define Mach_port_check(NAME)   \
+  void _Mach_port_check_##NAME(char *_unused[] __attribute__ ((unused))) \
+  { \
+  if (MACH_PORT_VALID (NAME))   \
+__print_fail_backtrace (#NAME " leaked",\
+__FILE__, __LINE__, "Port leak detector");  \
+  } \
+  char _Mach_port_check_x_##NAME[0] \
+  __attribute__ ((unused, cleanup (_Mach_port_check_##NAME)))
+
+#define Mach_server_success(ERR)   ((ERR) == 0 || (ERR) == MIG_NO_REPLY)
+
+#define Mach_server_port_check(ERR, NAME)   \
+  void _Mach_port_check_##NAME(char *_unused[] __attribute__ ((unused))) \
+  { \
+if (Mach_server_success (ERR) && MACH_PORT_VALID (NAME))\
+  __print_fail_backtrace (#NAME " leaked",  \
+  __FILE__, __LINE__, "Port leak detector"); \
+  } \
+  char _Mach_port_check_x_##NAME[0] \
+  __attribute__ ((unused, cleanup (_Mach_port_check_##NAME)))
+
+#endif /* __MACHX__ */
diff --git a/trans/crash.c b/trans/crash.c
index 0fe9b24..d911da3 100644
--- a/trans/crash.c
+++ b/trans/crash.c
@@ -21,6 +21,7 @@
along with the GNU Hurd; see the 

[PATCH hurd 1/2] xxx print fail

[Justus Winter]

---
 libshouldbeinlibc/assert-backtrace.c | 8 
 libshouldbeinlibc/assert-backtrace.h | 6 ++
 2 files changed, 14 insertions(+)

diff --git a/libshouldbeinlibc/assert-backtrace.c 
b/libshouldbeinlibc/assert-backtrace.c
index ca23c8d..72a49e9 100644
--- a/libshouldbeinlibc/assert-backtrace.c
+++ b/libshouldbeinlibc/assert-backtrace.c
@@ -76,4 +76,12 @@ __assert_perror_fail_backtrace (int errnum,
 
 }
 
+void
+__print_fail_backtrace (const char *message, const char *file,
+unsigned int line, const char *function)
+{
+  __assert_fail_base_backtrace ("%s: %s:%u: %s: %s.\n",
+   message, file, line, function);
+}
+
 #endif /* ! defined NDEBUG */
diff --git a/libshouldbeinlibc/assert-backtrace.h 
b/libshouldbeinlibc/assert-backtrace.h
index c54b810..b36e5b2 100644
--- a/libshouldbeinlibc/assert-backtrace.h
+++ b/libshouldbeinlibc/assert-backtrace.h
@@ -42,6 +42,12 @@ void __assert_perror_fail_backtrace (int errnum,
 const char *function)
   __attribute__ ((noreturn, unused));
 
+/* Likewise, but prints the given MESSAGE.  */
+void
+__print_fail_backtrace (const char *message, const char *file,
+unsigned int line, const char *function)
+  __attribute__ ((noreturn, unused));
+
 #define assert_backtrace(expr) \
   ((expr)  \
? (void) 0  \
-- 
2.1.4

RFC: Runtime checking of port handling

[Justus Winter]

Hello :)

tl;dr: Compiler-assisted runtime checking of port handling in
variables with automatic storage duration.  Do we want to go there?

Long version:

I recently found a pretty bad resource leak in our crash server and
pondered how to find such issues proactively.  The main idea is to 1/
clear variables if the right is transferred, and to 2/ check if it is
indeed cleared when the variable goes out of scope.

1/ can be idiomatically achieved using variants of
mach_port_deallocate that do error checking and clear the variable
holding the port name.

2/ can be done in a robust way using the gcc variable attribute
'cleanup' that executes a function when the variable goes out of
scope.

The follow up patch demonstrates the use of this in the crash servers
'S_crash_dump_task' server function.  Using it I already found a bug,
my initial attempt of fixing the resource leak in crash was
incomplete, and in fact leaking 'ctty_id' when suspending the crashing
task.

I believe this approach offers strong guarantees: It produces no false
negatives (if used correctly...), with acceptable manual work applying
it to existing code, and minimal runtime overhead (which can be
reduced to zero for NDEBUG builds).  Printing meaningful error
messages with backtraces will help both fixing bugs and deploying this
mechanism.

Do we want to do this, or is this too clever for its own good?

Cheers,
Justus