Wireshark · Ethereal-dev: [Ethereal-dev] Exception bugs

[Richard van der Hoff <richardv@xxxxxxxxxxxxx>]

Hi,

I've been trying to make ethereal's exception handling work, and, in
particular, make use of a FINALLY block.

As a Java programmer, I expected a FINALLY block would /always/ be
executed. This doesn't seem to be the case - indeed, there doesn't seem
to be any difference between a FINALLY block and the end of a TRY block
in the current implementation.

I then discovered that there aren't any dissectors using FINALLY at the
moment. Perhaps this is because it doesn't work :).

Going on to think about how this could be fixed, I also noticed a few
other things which jar (if you'll excuse the pun) with my Java-bred
expectations. In summary:

- FINALLY blocks not executed on exceptions (caught, uncaught, or
(re)thrown from a CATCH block)

- Exceptions not caught by a CATCH block aren't propagated to the parent
TRY, CATCH - instead they are just dropped. Hence unexpected exceptions
are sometimes ignored.

- throwing a new exception from a CATCH or FINALLY block re-enters the
CATCH block; this could produce an infinite loop.

I've attached a patch, exntest.patch, which adds a small stand-alone
test program which aims to test some of the exception functionality, and
exposes some of these problems.

The uselessness of FINALLY, in particular, seemed worth fixing, and I
think that dropping unexpected exceptions is also quite dangerous. I've
therefore created a second patch, exceptions.patch, which reworks the
macros in exceptions.h to address the above problems. I'm sorry it's a
bit of a rewrite - however, fixing the existing code looked pretty tough.

The only problem with this is that it exposes a bug in packet-frame.c
... so the third and final patch here, frame.patch, fixes that.

Comments are welcome on the patches - or even my interpretation of how
exception-handling ought to work. I'm away from my desk for the next few
days, however, so apologies if I don't reply immediately.

Thanks,

Richard

--
Richard van der Hoff <richardv@xxxxxxxxxxxxx>
Systems Analyst
Tel: +44 (0) 845 666 7778
http://www.mxtelecom.com

Index: epan/exceptions.h
===================================================================
RCS file: /cvs/ethereal/epan/exceptions.h,v
retrieving revision 1.1.1.2
retrieving revision 1.5
diff -u -r1.1.1.2 -r1.5
--- epan/exceptions.h	4 Jul 2005 18:17:28 -0000	1.1.1.2
+++ epan/exceptions.h	28 Jul 2005 14:49:23 -0000	1.5
@@ -66,23 +66,31 @@
  * This is really something like:
  *
  * {
- * 	x = setjmp()
+ * 	caught = FALSE:
+ * 	x = setjmp();
  * 	if (x == 0) {
  * 		<TRY code>
  * 	}
- * 	else if (x == 1) {
+ * 	if (!caught && x == 1) {
+ * 		caught = TRUE;
  * 		<CATCH(1) code>
  * 	}
- * 	else if (x == 2) {
+ * 	if (!caught && x == 2) {
+ * 		caught = TRUE;
  * 		<CATCH(2) code>
  * 	}
- * 	else if (x == 3 || x == 4) {
+ * 	if (!caught && (x == 3 || x == 4)) {
+ * 		caught = TRUE;
  * 		<CATCH2(3,4) code>
  * 	}
- * 	else {
- * 		<CATCH_ALL code> {
+ * 	if (!caught && x != 0) {
+ *		caught = TRUE;
+ * 		<CATCH_ALL code>
  * 	}
  * 	<FINALLY code>
+ * 	if(!caught) {
+ *      	RETHROW(x)
+ * 	}
  * }<ENDTRY tag>
  *
  * All CATCH's must precede a CATCH_ALL.
@@ -110,40 +118,65 @@
  * CLEANUP_CB_CALL_AND_POP
  */
 
+/* we do up to three passes through the bit of code after except_try_push(),
+ * and except_state is used to keep track of where we are.
+ */
+#define EXCEPT_CAUGHT   1 /* exception has been caught, no need to rethrow at
+                           * END_TRY */
 
+#define EXCEPT_RETHROWN 2 /* the exception was rethrown from a CATCH
+                           * block. Don't reenter the CATCH blocks, but do
+                           * execute FINALLY and rethrow at END_TRY */
+
+#define EXCEPT_FINALLY  4 /* we've entered the FINALLY block - don't allow
+                           * RETHROW, and don't reenter FINALLY if a
+                           * different exception is thrown */
 
 #define TRY \
 {\
 	except_t *exc; \
+	volatile int except_state = 0; \
 	static const except_id_t catch_spec[] = { \
 		{ XCEPT_GROUP_ETHEREAL, XCEPT_CODE_ANY } }; \
 	except_try_push(catch_spec, 1, &exc); \
-	if (exc == 0) { \
+	                                               \
+    	if(except_state & EXCEPT_CAUGHT)               \
+            except_state |= EXCEPT_RETHROWN;           \
+	except_state &= ~EXCEPT_CAUGHT;                \
+	                                               \
+	if (except_state == 0 && exc == 0)             \
 		/* user's code goes here */
 
 #define ENDTRY \
-	} \
+	/* rethrow the exception if necessary */ \
+	if(!(except_state&EXCEPT_CAUGHT) && exc != 0)  \
+	    except_rethrow(exc);                 \
 	except_try_pop();\
 }
 
+/* the (except_state |= EXCEPT_CAUGHT) in the below is a way of setting
+ * except_state before the user's code, without disrupting the user's code if
+ * it's a one-liner.
+ */
 #define CATCH(x) \
-	} \
-	else if (exc->except_id.except_code == (x)) { \
+	if (except_state == 0 && exc != 0 && exc->except_id.except_code == (x) && \
+	    (except_state |= EXCEPT_CAUGHT))                                      \
 		/* user's code goes here */
 
 #define CATCH2(x,y) \
-	} \
-	else if (exc->except_id.except_code == (x) || exc->except_id.except_code == (y)) { \
+	if (except_state == 0 && exc != 0 && \
+	    (exc->except_id.except_code == (x) || exc->except_id.except_code == (y)) && \
+	    (except_state|=EXCEPT_CAUGHT))                                             \
 		/* user's code goes here */
 
 #define CATCH_ALL \
-	} \
-	else { \
+	if (except_state == 0 && exc != 0 && \
+	    (except_state|=EXCEPT_CAUGHT))                                             \
 		/* user's code goes here */
 
+
 #define FINALLY \
-	} \
-	{ \
+	if( !(except_state & EXCEPT_FINALLY) && (except_state|=EXCEPT_FINALLY)) \
 		/* user's code goes here */
 
 #define THROW(x) \
@@ -154,7 +187,24 @@
 
 #define GET_MESSAGE			except_message(exc)
 
-#define RETHROW				except_rethrow(exc)
+#define RETHROW                                     \
+    {                                               \
+        /* check we're in a catch block */          \
+        g_assert(except_state == EXCEPT_CAUGHT);    \
+	/* we can't use except_rethrow here, as that pops a catch block \
+	 * off the stack, and we don't want to do that, because we want to \
+	 * excecute the FINALLY {} block first.     \
+	 * except_throw doesn't provide an interface to rethrow an existing \
+	 * exception; however, longjmping back to except_try_push() has the \
+	 * desired effect.			    \
+	 *					    \
+	 * Note also that THROW and RETHROW should provide much the same \
+	 * functionality in terms of which blocks to enter, so any messing \ 
+	 * about with except_state in here would indicate that THROW is \
+	 * doing the wrong thing.                   \
+	 */					    \
+        longjmp(except_ch.except_jmp,1);            \
+    }
 
 #define EXCEPT_CODE			except_code(exc)
 

Index: epan/Makefile.am
===================================================================
RCS file: /cvs/ethereal/epan/Makefile.am,v
retrieving revision 1.1.1.7
retrieving revision 1.8
diff -u -r1.1.1.7 -r1.8
--- epan/Makefile.am	4 Jul 2005 18:17:28 -0000	1.1.1.7
+++ epan/Makefile.am	27 Jul 2005 22:42:28 -0000	1.8
@@ -54,6 +54,7 @@
 	Makefile.common	\
 	Makefile.nmake	\
 	tvbtest.c	\
+	exntest.c	\
 	doxygen.cfg.in
 
 CLEANFILES = \
@@ -68,7 +69,12 @@
 libethereal_la_DEPENDENCIES = @G_ASCII_STRTOULL_LO@ @INET_ATON_LO@ @INET_PTON_LO@ @INET_NTOP_LO@ dfilter/libdfilter.la ftypes/libftypes.la dissectors/libdissectors.la
 
 tvbtest: tvbtest.o tvbuff.o except.o strutil.o
-	$(LINK) -o tvbtest tvbtest.o tvbuff.o except.o strutil.o `glib-config --libs`
+	$(LINK) $^ $(GLIB_LIBS) -lz
+
+exntest: exntest.o except.o
+	$(LINK) $^ $(GLIB_LIBS)
+
+tvbtest.o exntest.o: exceptions.h
 
 
 if HAVE_PLUGINS
Index: epan/exntest.c
===================================================================
RCS file: epan/exntest.c
diff -N epan/exntest.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ epan/exntest.c	28 Jul 2005 14:37:14 -0000
@@ -0,0 +1,202 @@
+/* Standalone program to test functionality of exceptions.
+ *
+ * $Id: exntest.c,v 1.1 2005/07/27 22:42:28 richardv Exp $
+ *
+ * Copyright (c) 2004 MX Telecom Ltd. <richardv@xxxxxxxxxxxxx>
+ *
+ * This program 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
+ * of the License, or (at your option) any later version.
+ *
+ * This program 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 this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA  02111-1307, USA.
+ *
+ */
+
+#include <stdio.h>
+#include <glib.h>
+#include "exceptions.h"
+
+gboolean failed = FALSE;
+
+void
+run_tests(void)
+{
+    volatile unsigned int ex_thrown, finally_called;
+
+    /* check that the right catch, and the finally, are called, on exception */
+    ex_thrown = finally_called = 0;
+    TRY {
+        THROW(BoundsError);
+    }
+    CATCH(BoundsError) {
+        ex_thrown++;
+    }
+    CATCH(ReportedBoundsError) {
+        printf("01: Caught wrong exception: ReportedBoundsError\n");
+        failed = TRUE;
+    }
+    CATCH_ALL {
+        printf("01: Caught wrong exception: %lu\n", exc->except_id.except_code);
+        failed = TRUE;
+    }
+    FINALLY {
+        finally_called ++;
+    }
+    ENDTRY;
+
+    if (ex_thrown != 1) {
+        printf("01: %u BoundsErrors (not 1) on caught exception\n", ex_thrown);
+        failed = TRUE;
+    }
+
+    if (finally_called != 1) {
+        printf("01: FINALLY called %u times (not 1) on caught exception\n", finally_called);
+        failed = TRUE;
+    }
+
+
+    /* check that no catch at all is called when there is no exn */
+    ex_thrown = finally_called = 0;
+    TRY {
+    }
+    CATCH(BoundsError) {
+        printf("02: Caught wrong exception: BoundsError\n");
+        failed = TRUE;
+    }
+    CATCH(ReportedBoundsError) {
+        printf("02: Caught wrong exception: ReportedBoundsError\n");
+        failed = TRUE;
+    }
+    CATCH_ALL {
+        printf("02: Caught wrong exception: %lu\n", exc->except_id.except_code);
+        failed = TRUE;
+    }
+    FINALLY {
+        finally_called ++;
+    }
+    ENDTRY;
+
+    if (finally_called != 1) {
+        printf("02: FINALLY called %u times (not 1) on no exception\n", finally_called);
+        failed = TRUE;
+    }
+
+
+    /* check that finally is called on an uncaught exception */
+    ex_thrown = finally_called = 0;
+    TRY {
+        TRY {
+            THROW(BoundsError);
+        }
+        FINALLY {
+            finally_called ++;
+        }
+        ENDTRY;
+    }
+    CATCH(BoundsError) {
+        ex_thrown++;
+    }
+    ENDTRY;
+    
+    if (finally_called != 1) {
+        printf("03: FINALLY called %u times (not 1) on uncaught exception\n", finally_called);
+        failed = TRUE;
+    }
+
+    if (ex_thrown != 1) {
+        printf("03: %u BoundsErrors (not 1) on uncaught exception\n", ex_thrown);
+        failed = TRUE;
+    }
+
+
+    /* check that finally is called on an rethrown exception */
+    ex_thrown = finally_called = 0;
+    TRY {
+        TRY {
+            THROW(BoundsError);
+        }
+        CATCH_ALL {
+            ex_thrown += 10;
+            RETHROW;
+        }
+        FINALLY {
+            finally_called += 10;
+        }
+        ENDTRY;
+    }
+    CATCH(BoundsError) {
+        ex_thrown ++;
+    }
+    FINALLY {
+        finally_called ++;
+    }
+    ENDTRY;
+    
+    if (finally_called != 11) {
+        printf("04: finally_called = %u (not 11) on rethrown exception\n", finally_called);
+        failed = TRUE;
+    }
+
+    if (ex_thrown != 11) {
+        printf("04: %u BoundsErrors (not 11) on rethrown exception\n", ex_thrown);
+        failed = TRUE;
+    }
+
+
+    /* check that finally is called on an exception thrown from a CATCH block */
+    ex_thrown = finally_called = 0;
+    TRY {
+        TRY {
+            THROW(BoundsError);
+        }
+        CATCH_ALL {
+            if(ex_thrown > 0) {
+                printf("05: Looping exception\n");
+                failed = TRUE;
+            } else {
+                ex_thrown += 10;
+                THROW(BoundsError);
+            }
+        }
+        FINALLY {
+            finally_called += 10;
+        }
+        ENDTRY;
+    }
+    CATCH(BoundsError) {
+        ex_thrown ++;
+    }
+    FINALLY {
+        finally_called ++;
+    }
+    ENDTRY;
+    
+    if (finally_called != 11) {
+        printf("05: finally_called = %u (not 11) on exception thrown from CATCH\n", finally_called);
+        failed = TRUE;
+    }
+
+    if (ex_thrown != 11) {
+        printf("05: %u BoundsErrors (not 11) on exception thrown from CATCH\n", ex_thrown);
+        failed = TRUE;
+    }
+
+    if(failed == FALSE )
+        printf("success\n");
+}
+
+int main(void)
+{
+    except_init();
+    run_tests();
+    except_deinit();
+    exit(failed?1:0);
+}

Index: epan/dissectors/packet-frame.c
===================================================================
RCS file: /cvs/ethereal/epan/dissectors/packet-frame.c,v
retrieving revision 1.1.1.2
retrieving revision 1.2
diff -u -r1.1.1.2 -r1.2
--- epan/dissectors/packet-frame.c	4 Jul 2005 18:17:31 -0000	1.1.1.2
+++ epan/dissectors/packet-frame.c	27 Jul 2005 22:30:16 -0000	1.2
@@ -198,6 +198,15 @@
 		0, 0, cap_len, "Capture Length: %d byte%s", cap_len,
 		plurality(cap_len, "", "s"));
 
+      /* we are going to be using proto_item_append_string() on
+       * hf_frame_protocols, and we must therefore disable the
+       * TRY_TO_FAKE_THIS_ITEM() optimisation for the tree by
+       * setting it as visible.
+       *
+       * See proto.h for details.
+       */
+      PTREE_DATA(fh_tree)->visible=1;
+
 	  ti = proto_tree_add_string(fh_tree, hf_frame_protocols, tvb,
 	  	0, 0, "");
       PROTO_ITEM_SET_GENERATED(ti);