From 9287813da1ae9076f29be111674d1795bee66447 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 1 Apr 2019 11:54:23 -0700 Subject: [PATCH] Fix union Lisp_Fwd * alignment bug It's not portable to cast (e.g.) struct Lisp_Objfwd * to union Lisp_Fwd * and then back again, because the compiler can then assume that the pointer is aligned for union Lisp_Fwd * when accessing the struct Lisp_Objfwd * components, and this assumption might be incorrect becase we don't force that alignment. * src/lisp.h (lispfwd): New type, replacing ... (union Lisp_Fwd): ... this type, which was removed. All uses changed. (SET_SYMBOL_FWD): 2nd arg is now void *, not lispfwd. All uses changed (casts no longer needed; they were not portable anyway). --- src/buffer.c | 8 +++--- src/data.c | 74 ++++++++++++++++++++++++++++----------------------- src/lisp.h | 41 ++++++++++++++-------------- src/lread.c | 8 +++--- src/pdumper.c | 26 +++++++++--------- 5 files changed, 80 insertions(+), 77 deletions(-) diff --git a/src/buffer.c b/src/buffer.c index 7c4691e52c0..c0f7521c9e1 100644 --- a/src/buffer.c +++ b/src/buffer.c @@ -1207,7 +1207,7 @@ buffer_local_value (Lisp_Object variable, Lisp_Object buffer) result = Fassoc (variable, BVAR (buf, local_var_alist), Qnil); if (!NILP (result)) { - if (blv->fwd) + if (blv->fwd.fwdptr) { /* What binding is loaded right now? */ Lisp_Object current_alist_element = blv->valcell; @@ -1228,7 +1228,7 @@ buffer_local_value (Lisp_Object variable, Lisp_Object buffer) } case SYMBOL_FORWARDED: { - union Lisp_Fwd *fwd = SYMBOL_FWD (sym); + lispfwd fwd = SYMBOL_FWD (sym); if (BUFFER_OBJFWDP (fwd)) result = per_buffer_value (buf, XBUFFER_OBJFWD (fwd)->offset); else @@ -2140,7 +2140,7 @@ void set_buffer_internal_2 (register struct buffer *b) Lisp_Object var = XCAR (XCAR (tail)); struct Lisp_Symbol *sym = XSYMBOL (var); if (sym->u.s.redirect == SYMBOL_LOCALIZED /* Just to be sure. */ - && SYMBOL_BLV (sym)->fwd) + && SYMBOL_BLV (sym)->fwd.fwdptr) /* Just reference the variable to cause it to become set for this buffer. */ Fsymbol_value (var); @@ -5444,7 +5444,7 @@ defvar_per_buffer (struct Lisp_Buffer_Objfwd *bo_fwd, const char *namestring, bo_fwd->predicate = predicate; sym->u.s.declared_special = true; sym->u.s.redirect = SYMBOL_FORWARDED; - SET_SYMBOL_FWD (sym, (union Lisp_Fwd *) bo_fwd); + SET_SYMBOL_FWD (sym, bo_fwd); XSETSYMBOL (PER_BUFFER_SYMBOL (offset), sym); if (PER_BUFFER_IDX (offset) == 0) diff --git a/src/data.c b/src/data.c index 30c578dee94..936bb74f69a 100644 --- a/src/data.c +++ b/src/data.c @@ -42,49 +42,49 @@ static void swap_in_symval_forwarding (struct Lisp_Symbol *, struct Lisp_Buffer_Local_Value *); static bool -BOOLFWDP (union Lisp_Fwd *a) +BOOLFWDP (lispfwd a) { return XFWDTYPE (a) == Lisp_Fwd_Bool; } static bool -INTFWDP (union Lisp_Fwd *a) +INTFWDP (lispfwd a) { return XFWDTYPE (a) == Lisp_Fwd_Int; } static bool -KBOARD_OBJFWDP (union Lisp_Fwd *a) +KBOARD_OBJFWDP (lispfwd a) { return XFWDTYPE (a) == Lisp_Fwd_Kboard_Obj; } static bool -OBJFWDP (union Lisp_Fwd *a) +OBJFWDP (lispfwd a) { return XFWDTYPE (a) == Lisp_Fwd_Obj; } static struct Lisp_Boolfwd * -XBOOLFWD (union Lisp_Fwd *a) +XBOOLFWD (lispfwd a) { eassert (BOOLFWDP (a)); - return &a->u_boolfwd; + return a.fwdptr; } static struct Lisp_Kboard_Objfwd * -XKBOARD_OBJFWD (union Lisp_Fwd *a) +XKBOARD_OBJFWD (lispfwd a) { eassert (KBOARD_OBJFWDP (a)); - return &a->u_kboard_objfwd; + return a.fwdptr; } static struct Lisp_Intfwd * -XFIXNUMFWD (union Lisp_Fwd *a) +XFIXNUMFWD (lispfwd a) { eassert (INTFWDP (a)); - return &a->u_intfwd; + return a.fwdptr; } static struct Lisp_Objfwd * -XOBJFWD (union Lisp_Fwd *a) +XOBJFWD (lispfwd a) { eassert (OBJFWDP (a)); - return &a->u_objfwd; + return a.fwdptr; } static void @@ -669,7 +669,7 @@ global value outside of any lexical scope. */) case SYMBOL_LOCALIZED: { struct Lisp_Buffer_Local_Value *blv = SYMBOL_BLV (sym); - if (blv->fwd) + if (blv->fwd.fwdptr) /* In set_internal, we un-forward vars when their value is set to Qunbound. */ return Qt; @@ -980,7 +980,7 @@ chain of aliases, signal a `cyclic-variable-indirection' error. */) swap_in_symval_forwarding for that. */ Lisp_Object -do_symval_forwarding (union Lisp_Fwd *valcontents) +do_symval_forwarding (lispfwd valcontents) { switch (XFWDTYPE (valcontents)) { @@ -1071,7 +1071,8 @@ wrong_range (Lisp_Object min, Lisp_Object max, Lisp_Object wrong) current buffer. This only plays a role for per-buffer variables. */ static void -store_symval_forwarding (union Lisp_Fwd *valcontents, register Lisp_Object newval, struct buffer *buf) +store_symval_forwarding (lispfwd valcontents, Lisp_Object newval, + struct buffer *buf) { switch (XFWDTYPE (valcontents)) { @@ -1178,12 +1179,12 @@ swap_in_global_binding (struct Lisp_Symbol *symbol) struct Lisp_Buffer_Local_Value *blv = SYMBOL_BLV (symbol); /* Unload the previously loaded binding. */ - if (blv->fwd) + if (blv->fwd.fwdptr) set_blv_value (blv, do_symval_forwarding (blv->fwd)); /* Select the global binding in the symbol. */ set_blv_valcell (blv, blv->defcell); - if (blv->fwd) + if (blv->fwd.fwdptr) store_symval_forwarding (blv->fwd, XCDR (blv->defcell), NULL); /* Indicate that the global binding is set up now. */ @@ -1213,7 +1214,7 @@ swap_in_symval_forwarding (struct Lisp_Symbol *symbol, struct Lisp_Buffer_Local_ /* Unload the previously loaded binding. */ tem1 = blv->valcell; - if (blv->fwd) + if (blv->fwd.fwdptr) set_blv_value (blv, do_symval_forwarding (blv->fwd)); /* Choose the new binding. */ { @@ -1227,7 +1228,7 @@ swap_in_symval_forwarding (struct Lisp_Symbol *symbol, struct Lisp_Buffer_Local_ /* Load the new binding. */ set_blv_valcell (blv, tem1); - if (blv->fwd) + if (blv->fwd.fwdptr) store_symval_forwarding (blv->fwd, blv_value (blv), NULL); } } @@ -1255,7 +1256,9 @@ find_symbol_value (Lisp_Object symbol) { struct Lisp_Buffer_Local_Value *blv = SYMBOL_BLV (sym); swap_in_symval_forwarding (sym, blv); - return blv->fwd ? do_symval_forwarding (blv->fwd) : blv_value (blv); + return (blv->fwd.fwdptr + ? do_symval_forwarding (blv->fwd) + : blv_value (blv)); } case SYMBOL_FORWARDED: return do_symval_forwarding (SYMBOL_FWD (sym)); @@ -1357,7 +1360,7 @@ set_internal (Lisp_Object symbol, Lisp_Object newval, Lisp_Object where, We need to unload it, and choose a new binding. */ /* Write out `realvalue' to the old loaded binding. */ - if (blv->fwd) + if (blv->fwd.fwdptr) set_blv_value (blv, do_symval_forwarding (blv->fwd)); /* Find the new binding. */ @@ -1404,12 +1407,12 @@ set_internal (Lisp_Object symbol, Lisp_Object newval, Lisp_Object where, /* Store the new value in the cons cell. */ set_blv_value (blv, newval); - if (blv->fwd) + if (blv->fwd.fwdptr) { if (voide) /* If storing void (making the symbol void), forward only through buffer-local indicator, not through Lisp_Objfwd, etc. */ - blv->fwd = NULL; + blv->fwd.fwdptr = NULL; else store_symval_forwarding (blv->fwd, newval, BUFFERP (where) @@ -1421,7 +1424,7 @@ set_internal (Lisp_Object symbol, Lisp_Object newval, Lisp_Object where, { struct buffer *buf = BUFFERP (where) ? XBUFFER (where) : current_buffer; - union Lisp_Fwd *innercontents = SYMBOL_FWD (sym); + lispfwd innercontents = SYMBOL_FWD (sym); if (BUFFER_OBJFWDP (innercontents)) { int offset = XBUFFER_OBJFWD (innercontents)->offset; @@ -1593,14 +1596,14 @@ default_value (Lisp_Object symbol) But the `realvalue' slot may be more up to date, since ordinary setq stores just that slot. So use that. */ struct Lisp_Buffer_Local_Value *blv = SYMBOL_BLV (sym); - if (blv->fwd && EQ (blv->valcell, blv->defcell)) + if (blv->fwd.fwdptr && EQ (blv->valcell, blv->defcell)) return do_symval_forwarding (blv->fwd); else return XCDR (blv->defcell); } case SYMBOL_FORWARDED: { - union Lisp_Fwd *valcontents = SYMBOL_FWD (sym); + lispfwd valcontents = SYMBOL_FWD (sym); /* For a built-in buffer-local variable, get the default value rather than letting do_symval_forwarding get the current value. */ @@ -1688,13 +1691,13 @@ set_default_internal (Lisp_Object symbol, Lisp_Object value, XSETCDR (blv->defcell, value); /* If the default binding is now loaded, set the REALVALUE slot too. */ - if (blv->fwd && EQ (blv->defcell, blv->valcell)) + if (blv->fwd.fwdptr && EQ (blv->defcell, blv->valcell)) store_symval_forwarding (blv->fwd, value, NULL); return; } case SYMBOL_FORWARDED: { - union Lisp_Fwd *valcontents = SYMBOL_FWD (sym); + lispfwd valcontents = SYMBOL_FWD (sym); /* Handle variables like case-fold-search that have special slots in the buffer. @@ -1750,7 +1753,7 @@ for this variable. */) union Lisp_Val_Fwd { Lisp_Object value; - union Lisp_Fwd *fwd; + lispfwd fwd; }; static struct Lisp_Buffer_Local_Value * @@ -1770,7 +1773,10 @@ make_blv (struct Lisp_Symbol *sym, bool forwarded, or keyboard-local forwarding. */ eassert (!(forwarded && BUFFER_OBJFWDP (valcontents.fwd))); eassert (!(forwarded && KBOARD_OBJFWDP (valcontents.fwd))); - blv->fwd = forwarded ? valcontents.fwd : NULL; + if (forwarded) + blv->fwd = valcontents.fwd; + else + blv->fwd.fwdptr = NULL; set_blv_where (blv, Qnil); blv->local_if_set = 0; set_blv_defcell (blv, tem); @@ -1941,7 +1947,7 @@ Instead, use `add-hook' and specify t for the LOCAL argument. */) Otherwise, if C code modifies the variable before we load the binding in, then that new value would clobber the default binding the next time we unload it. See bug#34318. */ - if (blv->fwd) + if (blv->fwd.fwdptr) swap_in_symval_forwarding (sym, blv); } @@ -1968,7 +1974,7 @@ From now on the default value will apply in this buffer. Return VARIABLE. */) case SYMBOL_PLAINVAL: return variable; case SYMBOL_FORWARDED: { - union Lisp_Fwd *valcontents = SYMBOL_FWD (sym); + lispfwd valcontents = SYMBOL_FWD (sym); if (BUFFER_OBJFWDP (valcontents)) { int offset = XBUFFER_OBJFWD (valcontents)->offset; @@ -2051,7 +2057,7 @@ BUFFER defaults to the current buffer. */) } case SYMBOL_FORWARDED: { - union Lisp_Fwd *valcontents = SYMBOL_FWD (sym); + lispfwd valcontents = SYMBOL_FWD (sym); if (BUFFER_OBJFWDP (valcontents)) { int offset = XBUFFER_OBJFWD (valcontents)->offset; @@ -2122,7 +2128,7 @@ If the current binding is global (the default), the value is nil. */) case SYMBOL_PLAINVAL: return Qnil; case SYMBOL_FORWARDED: { - union Lisp_Fwd *valcontents = SYMBOL_FWD (sym); + lispfwd valcontents = SYMBOL_FWD (sym); if (KBOARD_OBJFWDP (valcontents)) return Fframe_terminal (selected_frame); else if (!BUFFER_OBJFWDP (valcontents)) diff --git a/src/lisp.h b/src/lisp.h index 178eebed2a5..62c3230a148 100644 --- a/src/lisp.h +++ b/src/lisp.h @@ -797,6 +797,13 @@ INLINE void #define XUNTAG(a, type, ctype) ((ctype *) \ ((char *) XLP (a) - LISP_WORD_TAG (type))) + +/* A forwarding pointer to a value. It uses a generic pointer to + avoid alignment bugs that could occur if it used a pointer to a + union of the possible values (struct Lisp_Objfwd, struct + Lisp_Intfwd, etc.). The pointer is packaged inside a struct to + help static checking. */ +typedef struct { void *fwdptr; } lispfwd; /* Interned state of a symbol. */ @@ -862,7 +869,7 @@ struct Lisp_Symbol Lisp_Object value; struct Lisp_Symbol *alias; struct Lisp_Buffer_Local_Value *blv; - union Lisp_Fwd *fwd; + lispfwd fwd; } val; /* Function value of the symbol or Qnil if not fboundp. */ @@ -2171,10 +2178,10 @@ SYMBOL_BLV (struct Lisp_Symbol *sym) eassume (sym->u.s.redirect == SYMBOL_LOCALIZED && sym->u.s.val.blv); return sym->u.s.val.blv; } -INLINE union Lisp_Fwd * +INLINE lispfwd SYMBOL_FWD (struct Lisp_Symbol *sym) { - eassume (sym->u.s.redirect == SYMBOL_FORWARDED && sym->u.s.val.fwd); + eassume (sym->u.s.redirect == SYMBOL_FORWARDED && sym->u.s.val.fwd.fwdptr); return sym->u.s.val.fwd; } @@ -2197,10 +2204,10 @@ SET_SYMBOL_BLV (struct Lisp_Symbol *sym, struct Lisp_Buffer_Local_Value *v) sym->u.s.val.blv = v; } INLINE void -SET_SYMBOL_FWD (struct Lisp_Symbol *sym, union Lisp_Fwd *v) +SET_SYMBOL_FWD (struct Lisp_Symbol *sym, void *v) { eassume (sym->u.s.redirect == SYMBOL_FORWARDED && v); - sym->u.s.val.fwd = v; + sym->u.s.val.fwd.fwdptr = v; } INLINE Lisp_Object @@ -2727,7 +2734,7 @@ struct Lisp_Buffer_Local_Value Presumably equivalent to (defcell!=valcell). */ bool_bf found : 1; /* If non-NULL, a forwarding to the C var where it should also be set. */ - union Lisp_Fwd *fwd; /* Should never be (Buffer|Kboard)_Objfwd. */ + lispfwd fwd; /* Should never be (Buffer|Kboard)_Objfwd. */ /* The buffer for which the loaded binding was found. */ Lisp_Object where; /* A cons cell that holds the default value. It has the form @@ -2749,32 +2756,24 @@ struct Lisp_Kboard_Objfwd int offset; }; -union Lisp_Fwd - { - struct Lisp_Intfwd u_intfwd; - struct Lisp_Boolfwd u_boolfwd; - struct Lisp_Objfwd u_objfwd; - struct Lisp_Buffer_Objfwd u_buffer_objfwd; - struct Lisp_Kboard_Objfwd u_kboard_objfwd; - }; - INLINE enum Lisp_Fwd_Type -XFWDTYPE (union Lisp_Fwd *a) +XFWDTYPE (lispfwd a) { - return a->u_intfwd.type; + enum Lisp_Fwd_Type *p = a.fwdptr; + return *p; } INLINE bool -BUFFER_OBJFWDP (union Lisp_Fwd *a) +BUFFER_OBJFWDP (lispfwd a) { return XFWDTYPE (a) == Lisp_Fwd_Buffer_Obj; } INLINE struct Lisp_Buffer_Objfwd * -XBUFFER_OBJFWD (union Lisp_Fwd *a) +XBUFFER_OBJFWD (lispfwd a) { eassert (BUFFER_OBJFWDP (a)); - return &a->u_buffer_objfwd; + return a.fwdptr; } /* Lisp floating point type. */ @@ -3552,7 +3551,7 @@ extern _Noreturn void args_out_of_range (Lisp_Object, Lisp_Object); extern _Noreturn void args_out_of_range_3 (Lisp_Object, Lisp_Object, Lisp_Object); extern _Noreturn void circular_list (Lisp_Object); -extern Lisp_Object do_symval_forwarding (union Lisp_Fwd *); +extern Lisp_Object do_symval_forwarding (lispfwd); enum Set_Internal_Bind { SET_INTERNAL_SET, SET_INTERNAL_BIND, diff --git a/src/lread.c b/src/lread.c index 2d64b638ff5..dd35cf9063e 100644 --- a/src/lread.c +++ b/src/lread.c @@ -4434,7 +4434,7 @@ defvar_int (struct Lisp_Intfwd *i_fwd, i_fwd->intvar = address; XSYMBOL (sym)->u.s.declared_special = true; XSYMBOL (sym)->u.s.redirect = SYMBOL_FORWARDED; - SET_SYMBOL_FWD (XSYMBOL (sym), (union Lisp_Fwd *)i_fwd); + SET_SYMBOL_FWD (XSYMBOL (sym), i_fwd); } /* Similar but define a variable whose value is t if address contains 1, @@ -4449,7 +4449,7 @@ defvar_bool (struct Lisp_Boolfwd *b_fwd, b_fwd->boolvar = address; XSYMBOL (sym)->u.s.declared_special = true; XSYMBOL (sym)->u.s.redirect = SYMBOL_FORWARDED; - SET_SYMBOL_FWD (XSYMBOL (sym), (union Lisp_Fwd *)b_fwd); + SET_SYMBOL_FWD (XSYMBOL (sym), b_fwd); Vbyte_boolean_vars = Fcons (sym, Vbyte_boolean_vars); } @@ -4468,7 +4468,7 @@ defvar_lisp_nopro (struct Lisp_Objfwd *o_fwd, o_fwd->objvar = address; XSYMBOL (sym)->u.s.declared_special = true; XSYMBOL (sym)->u.s.redirect = SYMBOL_FORWARDED; - SET_SYMBOL_FWD (XSYMBOL (sym), (union Lisp_Fwd *)o_fwd); + SET_SYMBOL_FWD (XSYMBOL (sym), o_fwd); } void @@ -4492,7 +4492,7 @@ defvar_kboard (struct Lisp_Kboard_Objfwd *ko_fwd, ko_fwd->offset = offset; XSYMBOL (sym)->u.s.declared_special = true; XSYMBOL (sym)->u.s.redirect = SYMBOL_FORWARDED; - SET_SYMBOL_FWD (XSYMBOL (sym), (union Lisp_Fwd *)ko_fwd); + SET_SYMBOL_FWD (XSYMBOL (sym), ko_fwd); } /* Check that the elements of lpath exist. */ diff --git a/src/pdumper.c b/src/pdumper.c index a9b3732a2d4..53a10b62b3f 100644 --- a/src/pdumper.c +++ b/src/pdumper.c @@ -2334,32 +2334,30 @@ dump_fwd_kboard_obj (struct dump_context *ctx, } static dump_off -dump_fwd (struct dump_context *ctx, union Lisp_Fwd *fwd) +dump_fwd (struct dump_context *ctx, lispfwd fwd) { -#if CHECK_STRUCTS && !defined (HASH_Lisp_Fwd_5227B18E87) -# error "Lisp_Fwd changed. See CHECK_STRUCTS comment." -#endif #if CHECK_STRUCTS && !defined (HASH_Lisp_Fwd_Type_9CBA6EE55E) # error "Lisp_Fwd_Type changed. See CHECK_STRUCTS comment." #endif + void const *p = fwd.fwdptr; dump_off offset; switch (XFWDTYPE (fwd)) { case Lisp_Fwd_Int: - offset = dump_fwd_int (ctx, &fwd->u_intfwd); + offset = dump_fwd_int (ctx, p); break; case Lisp_Fwd_Bool: - offset = dump_fwd_bool (ctx, &fwd->u_boolfwd); + offset = dump_fwd_bool (ctx, p); break; case Lisp_Fwd_Obj: - offset = dump_fwd_obj (ctx, &fwd->u_objfwd); + offset = dump_fwd_obj (ctx, p); break; case Lisp_Fwd_Buffer_Obj: - offset = dump_fwd_buffer_obj (ctx, &fwd->u_buffer_objfwd); + offset = dump_fwd_buffer_obj (ctx, p); break; case Lisp_Fwd_Kboard_Obj: - offset = dump_fwd_kboard_obj (ctx, &fwd->u_kboard_objfwd); + offset = dump_fwd_kboard_obj (ctx, p); break; default: emacs_abort (); @@ -2372,20 +2370,20 @@ static dump_off dump_blv (struct dump_context *ctx, const struct Lisp_Buffer_Local_Value *blv) { -#if CHECK_STRUCTS && !defined (HASH_Lisp_Buffer_Local_Value_066F33A92E) +#if CHECK_STRUCTS && !defined HASH_Lisp_Buffer_Local_Value_3C363FAC3C # error "Lisp_Buffer_Local_Value changed. See CHECK_STRUCTS comment." #endif struct Lisp_Buffer_Local_Value out; dump_object_start (ctx, &out, sizeof (out)); DUMP_FIELD_COPY (&out, blv, local_if_set); DUMP_FIELD_COPY (&out, blv, found); - if (blv->fwd) - dump_field_fixup_later (ctx, &out, blv, &blv->fwd); + if (blv->fwd.fwdptr) + dump_field_fixup_later (ctx, &out, blv, &blv->fwd.fwdptr); dump_field_lv (ctx, &out, blv, &blv->where, WEIGHT_NORMAL); dump_field_lv (ctx, &out, blv, &blv->defcell, WEIGHT_STRONG); dump_field_lv (ctx, &out, blv, &blv->valcell, WEIGHT_STRONG); dump_off offset = dump_object_finish (ctx, &out, sizeof (out)); - if (blv->fwd) + if (blv->fwd.fwdptr) dump_remember_fixup_ptr_raw (ctx, offset + dump_offsetof (struct Lisp_Buffer_Local_Value, fwd), @@ -2437,7 +2435,7 @@ dump_symbol (struct dump_context *ctx, Lisp_Object object, dump_off offset) { -#if CHECK_STRUCTS && !defined (HASH_Lisp_Symbol_60EA1E748E) +#if CHECK_STRUCTS && !defined HASH_Lisp_Symbol_999DC26DEC # error "Lisp_Symbol changed. See CHECK_STRUCTS comment." #endif #if CHECK_STRUCTS && !defined (HASH_symbol_redirect_ADB4F5B113) -- 2.39.2