zsh-workers
 help / color / mirror / code / Atom feed
* 5.8: LTO exposes some new issues
@ 2020-07-21 23:41 Tomasz Kłoczko
  2020-07-22  5:59 ` Daniel Shahaf
  0 siblings, 1 reply; 20+ messages in thread
From: Tomasz Kłoczko @ 2020-07-21 23:41 UTC (permalink / raw)
  To: zsh-workers

[-- Attachment #1: Type: text/plain, Size: 3473 bytes --]

Hi

Looks like by using LTO is possible to expose new source code issues.
Also looks like zsh still is using  mktemp()

gcc -Wl,-z,relro -Wl,--as-needed  -Wl,-z,now
-specs=/usr/lib/rpm/redhat/redhat-hardened-ld -flto=auto
-flto-partition=none -fuse-linker-plugin  -rdynamic -o zsh main.o  `cat
stamp-modobjs`   -lpcre -ldl -lncursesw -ltinfo -lrt -lm  -lc
hist.epro:19:18: warning: type of 'histtab' does not match original
declaration [-Wlto-type-mismatch]
   19 | extern HashTable histtab;
      |                  ^
hist.c:101:11: note: 'histtab' was previously declared here
  101 | HashTable histtab;
      |           ^
hist.c:101:11: note: code may be misoptimized unless '-fno-strict-aliasing'
is used
hashtable.epro:21:38: warning: type of 'cmdnamtab' does not match original
declaration [-Wlto-type-mismatch]
   21 | extern mod_import_variable HashTable cmdnamtab;
      |                                      ^
hashtable.c:585:22: note: 'cmdnamtab' was previously declared here
  585 | mod_export HashTable cmdnamtab;
      |                      ^
hashtable.c:585:22: note: code may be misoptimized unless
'-fno-strict-aliasing' is used
hashtable.epro:33:38: warning: type of 'aliastab' does not match original
declaration [-Wlto-type-mismatch]
   33 | extern mod_import_variable HashTable aliastab;
      |                                      ^
hashtable.c:1172:22: note: 'aliastab' was previously declared here
 1172 | mod_export HashTable aliastab;
      |                      ^
hashtable.c:1172:22: note: code may be misoptimized unless
'-fno-strict-aliasing' is used
hashtable.epro:34:38: warning: type of 'sufaliastab' does not match
original declaration [-Wlto-type-mismatch]
   34 | extern mod_import_variable HashTable sufaliastab;
      |                                      ^
hashtable.c:1177:22: note: 'sufaliastab' was previously declared here
 1177 | mod_export HashTable sufaliastab;
      |                      ^
hashtable.c:1177:22: note: code may be misoptimized unless
'-fno-strict-aliasing' is used
hashtable.epro:31:38: warning: type of 'reswdtab' does not match original
declaration [-Wlto-type-mismatch]
   31 | extern mod_import_variable HashTable reswdtab;
      |                                      ^
hashtable.c:1109:22: note: 'reswdtab' was previously declared here
 1109 | mod_export HashTable reswdtab;
      |                      ^
hashtable.c:1109:22: note: code may be misoptimized unless
'-fno-strict-aliasing' is used
hashtable.epro:25:38: warning: type of 'shfunctab' does not match original
declaration [-Wlto-type-mismatch]
   25 | extern mod_import_variable HashTable shfunctab;
      |                                      ^
hashtable.c:803:22: note: 'shfunctab' was previously declared here
  803 | mod_export HashTable shfunctab;
      |                      ^
hashtable.c:803:22: note: code may be misoptimized unless
'-fno-strict-aliasing' is used
utils.c: In function 'getkeystring':
lto1: warning: function may return address of local variable
[-Wreturn-local-addr]
utils.c:6644:16: note: declared here
 6644 |     char *buf, tmp[1];
      |                ^
/usr/bin/ld: zsh.lto.o: in function `gettempname':
/home/tkloczko/rpmbuild/BUILD/zsh-5.8/Src/utils.c:2226: warning: the use of
`mktemp' is dangerous, better use `mkstemp' or `mkdtemp'

kloczek
-- 
Tomasz Kłoczko | LinkedIn: *http://lnkd.in/FXPWxH <http://lnkd.in/FXPWxH>*

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: 5.8: LTO exposes some new issues
  2020-07-21 23:41 5.8: LTO exposes some new issues Tomasz Kłoczko
@ 2020-07-22  5:59 ` Daniel Shahaf
  2020-07-25 17:43   ` Bart Schaefer
  0 siblings, 1 reply; 20+ messages in thread
From: Daniel Shahaf @ 2020-07-22  5:59 UTC (permalink / raw)
  To: Tomasz Kłoczko, zsh-workers

Tomasz Kłoczko wrote on Tue, 21 Jul 2020 23:41 +00:00:
> /usr/bin/ld: zsh.lto.o: in function `gettempname':
> /home/tkloczko/rpmbuild/BUILD/zsh-5.8/Src/utils.c:2226: warning: the use of
> `mktemp' is dangerous, better use `mkstemp' or `mkdtemp'

False positive; see comments around that line.

Haven't looked at the other warnings you reported.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: 5.8: LTO exposes some new issues
  2020-07-22  5:59 ` Daniel Shahaf
@ 2020-07-25 17:43   ` Bart Schaefer
       [not found]     ` <CABB28CxSD5w-SY-iCVYuQ4kJfBpNJOWhpk4HOrS1DNPfMVztgw@mail.gmail.com>
  0 siblings, 1 reply; 20+ messages in thread
From: Bart Schaefer @ 2020-07-25 17:43 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: Tomasz Kłoczko, zsh-workers

On Tue, Jul 21, 2020 at 11:00 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>
> Haven't looked at the other warnings you reported.

They all appear to be clashes where the same pointer is declared
"extern" in a header file but not so in the original source.  It looks
as if the "mod_import_variable" preprocessor definition is supposed to
deal with this, perhaps it's not being properly defined in the
situation where Tomasz is compiling.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Fwd: 5.8: LTO exposes some new issues
       [not found]     ` <CABB28CxSD5w-SY-iCVYuQ4kJfBpNJOWhpk4HOrS1DNPfMVztgw@mail.gmail.com>
@ 2020-07-25 20:05       ` Bart Schaefer
  2020-07-27  2:12         ` Daniel Shahaf
  0 siblings, 1 reply; 20+ messages in thread
From: Bart Schaefer @ 2020-07-25 20:05 UTC (permalink / raw)
  To: Zsh hackers list

I am not likely to pursue this further, I don't have a suitable build
environment.

---------- Forwarded message ---------
From: Tomasz Kłoczko <kloczko.tomasz@gmail.com>
Date: Sat, Jul 25, 2020 at 11:59 AM
Subject: Re: 5.8: LTO exposes some new issues
To: Bart Schaefer <schaefer@brasslantern.com>


On Sat, 25 Jul 2020 at 18:43, Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> On Tue, Jul 21, 2020 at 11:00 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> >
> > Haven't looked at the other warnings you reported.
>
> They all appear to be clashes where the same pointer is declared
> "extern" in a header file but not so in the original source.  It looks
> as if the "mod_import_variable" preprocessor definition is supposed to
> deal with this, perhaps it's not being properly defined in the
> situation where Tomasz is compiling.


Yes and they need to be resolved because even single such warnings and
ld abandons applying LTO.
If you will decide which version should be used in both locations and
will have some patch for that please let me know about that because I
can test that kind of patch instantly proving that there is no other
coalition with LTO.

kloczek
-- 
Tomasz Kłoczko | LinkedIn: http://lnkd.in/FXPWxH

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Fwd: 5.8: LTO exposes some new issues
  2020-07-25 20:05       ` Fwd: " Bart Schaefer
@ 2020-07-27  2:12         ` Daniel Shahaf
  2020-07-27 10:07           ` Tomasz Kłoczko
  0 siblings, 1 reply; 20+ messages in thread
From: Daniel Shahaf @ 2020-07-27  2:12 UTC (permalink / raw)
  To: Tomasz Kłoczko, Zsh hackers list

Tomasz, if you devise a patch for these issues, please post it so it can
be reviewed and applied.  (If you prefer to wait for the Internet to
write the patch you want, feel free to do so, but that strategy doesn't
always work.)

Cheers,

Daniel


Bart Schaefer wrote on Sat, 25 Jul 2020 20:05 +00:00:
> I am not likely to pursue this further, I don't have a suitable build
> environment.
> 
> ---------- Forwarded message ---------
> From: Tomasz Kłoczko <kloczko.tomasz@gmail.com>
> Date: Sat, Jul 25, 2020 at 11:59 AM
> Subject: Re: 5.8: LTO exposes some new issues
> To: Bart Schaefer <schaefer@brasslantern.com>
> 
> 
> On Sat, 25 Jul 2020 at 18:43, Bart Schaefer <schaefer@brasslantern.com> wrote:
> >
> > On Tue, Jul 21, 2020 at 11:00 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > >
> > > Haven't looked at the other warnings you reported.
> >
> > They all appear to be clashes where the same pointer is declared
> > "extern" in a header file but not so in the original source.  It looks
> > as if the "mod_import_variable" preprocessor definition is supposed to
> > deal with this, perhaps it's not being properly defined in the
> > situation where Tomasz is compiling.
> 
> 
> Yes and they need to be resolved because even single such warnings and
> ld abandons applying LTO.
> If you will decide which version should be used in both locations and
> will have some patch for that please let me know about that because I
> can test that kind of patch instantly proving that there is no other
> coalition with LTO.
> 
> kloczek
> -- 
> Tomasz Kłoczko | LinkedIn: http://lnkd.in/FXPWxH
>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Fwd: 5.8: LTO exposes some new issues
  2020-07-27  2:12         ` Daniel Shahaf
@ 2020-07-27 10:07           ` Tomasz Kłoczko
  2020-07-27 11:09             ` Roman Perepelitsa
  0 siblings, 1 reply; 20+ messages in thread
From: Tomasz Kłoczko @ 2020-07-27 10:07 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: Zsh hackers list

[-- Attachment #1: Type: text/plain, Size: 647 bytes --]

On Mon, 27 Jul 2020 at 03:12, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:

> Tomasz, if you devise a patch for these issues, please post it so it can
> be reviewed and applied.  (If you prefer to wait for the Internet to
> write the patch you want, feel free to do so, but that strategy doesn't
> always work.)
>

Don't get me wrong but I have no time to learn that code from scratch to
make a decision which one of each pair of definitions should be chosen as
the correct one.
For someone who is maintaining that code, making such a decision should be
way easier.

kloczek
-- 
Tomasz Kłoczko | LinkedIn: http://lnkd.in/FXPWxH

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Fwd: 5.8: LTO exposes some new issues
  2020-07-27 10:07           ` Tomasz Kłoczko
@ 2020-07-27 11:09             ` Roman Perepelitsa
  2020-07-27 12:19               ` Roman Perepelitsa
  0 siblings, 1 reply; 20+ messages in thread
From: Roman Perepelitsa @ 2020-07-27 11:09 UTC (permalink / raw)
  To: Tomasz Kłoczko; +Cc: Daniel Shahaf, Zsh hackers list

[-- Attachment #1: Type: text/plain, Size: 715 bytes --]

On Mon, Jul 27, 2020 at 12:08 PM Tomasz Kłoczko
<kloczko.tomasz@gmail.com> wrote:
>
> For someone who is maintaining that code, making such a decision should be
> way easier.

This statement is true but it's too general to be of any use here. Of
course making changes is easier for maintainers than for outsiders.
This doesn't mean that all changes (and this change in particular)
must be made by maintainers.

Anyway, I took a look at it. The problem is that `struct hashtable`
has a different set of members in different translation units. This is
undefined behavior whether LTO is used or not. Undefined behavior is
bad, so it would be nice to have this bug fixed. A patch is attached.

Roman.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 2182 bytes --]

diff --git a/Src/hashtable.c b/Src/hashtable.c
index e210ddeca..fa9d31052 100644
--- a/Src/hashtable.c
+++ b/Src/hashtable.c
@@ -28,23 +28,6 @@
  */
 
 #include "../config.h"
-
-#ifdef ZSH_HASH_DEBUG
-# define HASHTABLE_DEBUG_MEMBERS \
-    /* Members of struct hashtable used for debugging hash tables */ \
-    HashTable next, last;	/* linked list of all hash tables           */ \
-    char *tablename;		/* string containing name of the hash table */ \
-    PrintTableStats printinfo;	/* pointer to function to print table stats */
-#else /* !ZSH_HASH_DEBUG */
-# define HASHTABLE_DEBUG_MEMBERS
-#endif /* !ZSH_HASH_DEBUG */
-
-#define HASHTABLE_INTERNAL_MEMBERS \
-    ScanStatus scan;		/* status of a scan over this hashtable     */ \
-    HASHTABLE_DEBUG_MEMBERS
-
-typedef struct scanstatus *ScanStatus;
-
 #include "zsh.mdh"
 #include "hashtable.pro"
 
diff --git a/Src/zsh.h b/Src/zsh.h
index c48be4ffd..f9caa2f6e 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -1182,6 +1182,18 @@ typedef void (*PrintTableStats) _((HashTable));
 
 /* hash table for standard open hashing */
 
+#ifdef ZSH_HASH_DEBUG
+# define HASHTABLE_DEBUG_MEMBERS \
+    /* Members of struct hashtable used for debugging hash tables */ \
+    HashTable next, last;	/* linked list of all hash tables           */ \
+    char *tablename;		/* string containing name of the hash table */ \
+    PrintTableStats printinfo;	/* pointer to function to print table stats */
+#else /* !ZSH_HASH_DEBUG */
+# define HASHTABLE_DEBUG_MEMBERS
+#endif /* !ZSH_HASH_DEBUG */
+
+typedef struct scanstatus *ScanStatus;
+
 struct hashtable {
     /* HASHTABLE DATA */
     int hsize;			/* size of nodes[]  (number of hash values)   */
@@ -1205,9 +1217,9 @@ struct hashtable {
     ScanFunc printnode;		/* pointer to function to print a node        */
     ScanTabFunc scantab;	/* pointer to function to scan table          */
 
-#ifdef HASHTABLE_INTERNAL_MEMBERS
-    HASHTABLE_INTERNAL_MEMBERS	/* internal use in hashtable.c                */
-#endif
+    /* HASHTABLE INTERNAL MEMBERS */
+    ScanStatus scan;		/* status of a scan over this hashtable       */
+    HASHTABLE_DEBUG_MEMBERS
 };
 
 /* generic hash table node */

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Fwd: 5.8: LTO exposes some new issues
  2020-07-27 11:09             ` Roman Perepelitsa
@ 2020-07-27 12:19               ` Roman Perepelitsa
  2020-07-27 12:46                 ` Tomasz Kłoczko
  2020-07-28  7:53                 ` Daniel Shahaf
  0 siblings, 2 replies; 20+ messages in thread
From: Roman Perepelitsa @ 2020-07-27 12:19 UTC (permalink / raw)
  To: Tomasz Kłoczko; +Cc: Daniel Shahaf, Zsh hackers list

[-- Attachment #1: Type: text/plain, Size: 183 bytes --]

On Mon, Jul 27, 2020 at 1:09 PM Roman Perepelitsa
<roman.perepelitsa@gmail.com> wrote:
> A patch is attached.

Here's a cleaned up patch (HASHTABLE_DEBUG_MEMBERS is inlined).

Roman.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 2064 bytes --]

diff --git a/Src/hashtable.c b/Src/hashtable.c
index e210ddeca..fa9d31052 100644
--- a/Src/hashtable.c
+++ b/Src/hashtable.c
@@ -28,23 +28,6 @@
  */
 
 #include "../config.h"
-
-#ifdef ZSH_HASH_DEBUG
-# define HASHTABLE_DEBUG_MEMBERS \
-    /* Members of struct hashtable used for debugging hash tables */ \
-    HashTable next, last;	/* linked list of all hash tables           */ \
-    char *tablename;		/* string containing name of the hash table */ \
-    PrintTableStats printinfo;	/* pointer to function to print table stats */
-#else /* !ZSH_HASH_DEBUG */
-# define HASHTABLE_DEBUG_MEMBERS
-#endif /* !ZSH_HASH_DEBUG */
-
-#define HASHTABLE_INTERNAL_MEMBERS \
-    ScanStatus scan;		/* status of a scan over this hashtable     */ \
-    HASHTABLE_DEBUG_MEMBERS
-
-typedef struct scanstatus *ScanStatus;
-
 #include "zsh.mdh"
 #include "hashtable.pro"
 
diff --git a/Src/zsh.h b/Src/zsh.h
index c48be4ffd..4b2e1bdfc 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -1182,6 +1182,8 @@ typedef void (*PrintTableStats) _((HashTable));
 
 /* hash table for standard open hashing */
 
+typedef struct scanstatus *ScanStatus;
+
 struct hashtable {
     /* HASHTABLE DATA */
     int hsize;			/* size of nodes[]  (number of hash values)   */
@@ -1205,9 +1207,15 @@ struct hashtable {
     ScanFunc printnode;		/* pointer to function to print a node        */
     ScanTabFunc scantab;	/* pointer to function to scan table          */
 
-#ifdef HASHTABLE_INTERNAL_MEMBERS
-    HASHTABLE_INTERNAL_MEMBERS	/* internal use in hashtable.c                */
-#endif
+    /* HASHTABLE INTERNAL MEMBERS */
+    ScanStatus scan;		/* status of a scan over this hashtable       */
+
+#ifdef ZSH_HASH_DEBUG
+    /* Members of struct hashtable used for debugging hash tables           */ \
+    HashTable next, last;	/* linked list of all hash tables           */ \
+    char *tablename;		/* string containing name of the hash table */ \
+    PrintTableStats printinfo;	/* pointer to function to print table stats */
+#endif /* !ZSH_HASH_DEBUG */
 };
 
 /* generic hash table node */

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Fwd: 5.8: LTO exposes some new issues
  2020-07-27 12:19               ` Roman Perepelitsa
@ 2020-07-27 12:46                 ` Tomasz Kłoczko
  2020-07-27 14:13                   ` Roman Perepelitsa
                                     ` (2 more replies)
  2020-07-28  7:53                 ` Daniel Shahaf
  1 sibling, 3 replies; 20+ messages in thread
From: Tomasz Kłoczko @ 2020-07-27 12:46 UTC (permalink / raw)
  To: Roman Perepelitsa; +Cc: Daniel Shahaf, Zsh hackers list

[-- Attachment #1: Type: text/plain, Size: 1645 bytes --]

On Mon, 27 Jul 2020 at 13:20, Roman Perepelitsa <roman.perepelitsa@gmail.com>
wrote:

> On Mon, Jul 27, 2020 at 1:09 PM Roman Perepelitsa
> <roman.perepelitsa@gmail.com> wrote:
> > A patch is attached.
>
> Here's a cleaned up patch (HASHTABLE_DEBUG_MEMBERS is inlined).
>

Thank you very much for the patch.
All warnings about mismatching declarations gone however still one other
warning remains.
Here is whole list of compile and linking warnings:

exec.c: In function 'execcmd_fork':
exec.c:2774:2: warning: ignoring return value of 'nice' declared with
attribute 'warn_unused_result' [-Wunused-result]
 2774 |  nice(5);
      |  ^~~~~~~
utils.c: In function 'getkeystring':
lto1: warning: function may return address of local variable
[-Wreturn-local-addr]
utils.c:6644:16: note: declared here
 6644 |     char *buf, tmp[1];
      |                ^
/usr/bin/ld: zsh.lto.o: in function `gettempname':
/home/tkloczko/rpmbuild/BUILD/zsh-5.8/Src/utils.c:2226: warning: the use of
`mktemp' is dangerous, better use `mkstemp' or `mkdtemp'
utils.c: In function 'getkeystring':
lto1: warning: function may return address of local variable
[-Wreturn-local-addr]
utils.c:6644:16: note: declared here
 6644 |     char *buf, tmp[1];
      |                ^
/usr/bin/ld: zsh.lto.o: in function `gettempname':
/home/tkloczko/rpmbuild/BUILD/zsh-5.8/Src/utils.c:2226: warning: the use of
`mktemp' is dangerous, better use `mkstemp' or `mkdtemp'


Other topic: do you have any plans to move to pcre2? :P

One more time: thank you for you time :)

kloczek
-- 
Tomasz Kłoczko | LinkedIn: http://lnkd.in/FXPWxH

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Fwd: 5.8: LTO exposes some new issues
  2020-07-27 12:46                 ` Tomasz Kłoczko
@ 2020-07-27 14:13                   ` Roman Perepelitsa
  2020-07-27 14:19                   ` Roman Perepelitsa
  2020-07-28  8:19                   ` Daniel Shahaf
  2 siblings, 0 replies; 20+ messages in thread
From: Roman Perepelitsa @ 2020-07-27 14:13 UTC (permalink / raw)
  To: Tomasz Kłoczko; +Cc: Daniel Shahaf, Zsh hackers list

[-- Attachment #1: Type: text/plain, Size: 425 bytes --]

On Mon, Jul 27, 2020 at 2:47 PM Tomasz Kłoczko <kloczko.tomasz@gmail.com> wrote:
>
> Here is whole list of compile and linking warnings:
>
> exec.c: In function 'execcmd_fork':
> exec.c:2774:2: warning: ignoring return value of 'nice' declared with attribute 'warn_unused_result' [-Wunused-result]
>  2774 |  nice(5);
>       |  ^~~~~~~

This is a false positive. Patch to suppress the warning attached.

Roman.

[-- Attachment #2: suppress-nice-warn.txt --]
[-- Type: text/plain, Size: 446 bytes --]

diff --git a/Src/exec.c b/Src/exec.c
index 7120a2c34..ecad923de 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -2789,8 +2789,7 @@ execcmd_fork(Estate state, int how, int type, Wordcode varspc,
     /* Check if we should run background jobs at a lower priority. */
     if ((how & Z_ASYNC) && isset(BGNICE)) {
 	errno = 0;
-	nice(5);
-	if (errno)
+	if (nice(5) == -1 && errno)
 	    zwarn("nice(5) failed: %e", errno);
     }
 #endif /* HAVE_NICE */

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Fwd: 5.8: LTO exposes some new issues
  2020-07-27 12:46                 ` Tomasz Kłoczko
  2020-07-27 14:13                   ` Roman Perepelitsa
@ 2020-07-27 14:19                   ` Roman Perepelitsa
  2020-07-28  8:09                     ` Daniel Shahaf
  2020-07-28 10:55                     ` Fwd: " Roman Perepelitsa
  2020-07-28  8:19                   ` Daniel Shahaf
  2 siblings, 2 replies; 20+ messages in thread
From: Roman Perepelitsa @ 2020-07-27 14:19 UTC (permalink / raw)
  To: Tomasz Kłoczko; +Cc: Daniel Shahaf, Zsh hackers list

[-- Attachment #1: Type: text/plain, Size: 1111 bytes --]

On Mon, Jul 27, 2020 at 2:47 PM Tomasz Kłoczko <kloczko.tomasz@gmail.com> wrote:
> Here is whole list of compile and linking warnings:
>
> utils.c: In function 'getkeystring':
> lto1: warning: function may return address of local variable [-Wreturn-local-addr]
> utils.c:6644:16: note: declared here
>  6644 |     char *buf, tmp[1];
>       |                ^

This one might be a real bug. At the end of getkeystring there is an
explicit check for `how & GETKEY_SINGLE_CHAR`. If this condition is
true at that point, the code runs into undefined behavior. First,
writing to `*t` is illegal because it points outside of `tmp`. Second,
returning `buf` is illegal because it holds a pointer to a local
variable (hence the warning).

I'm attaching a patch that keeps the branch (although I'm not sure
it's reachable) and makes the code less broken if it ever triggers. I
cannot verify that it gets rid of the warning because I don't get this
warning with unmodified code.

FYI: I won't be doing anything about the warning in gettempname (which
I'm not getting with my toolchain).

Roman.

[-- Attachment #2: explicit-single-char-return.txt --]
[-- Type: text/plain, Size: 593 bytes --]

diff --git a/Src/utils.c b/Src/utils.c
index 5151b89a8..e03f41468 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -7162,11 +7162,13 @@ getkeystring(char *s, int *len, int how, int *misc)
      */
     DPUTS((how & (GETKEY_DOLLAR_QUOTE|GETKEY_UPDATE_OFFSET)) ==
 	  GETKEY_DOLLAR_QUOTE, "BUG: unterminated $' substitution");
+    if (how & GETKEY_SINGLE_CHAR) {
+	*misc = 0;
+	return s;
+    }
     *t = '\0';
     if (how & GETKEY_DOLLAR_QUOTE)
 	*tdest = '\0';
-    if (how & GETKEY_SINGLE_CHAR)
-	*misc = 0;
     else
 	*len = ((how & GETKEY_DOLLAR_QUOTE) ? tdest : t) - buf;
     return buf;

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: 5.8: LTO exposes some new issues
  2020-07-27 12:19               ` Roman Perepelitsa
  2020-07-27 12:46                 ` Tomasz Kłoczko
@ 2020-07-28  7:53                 ` Daniel Shahaf
  2020-07-28  8:25                   ` Peter Stephenson
  1 sibling, 1 reply; 20+ messages in thread
From: Daniel Shahaf @ 2020-07-28  7:53 UTC (permalink / raw)
  To: Roman Perepelitsa; +Cc: Tomasz Kłoczko, Zsh hackers list

Roman Perepelitsa wrote on Mon, 27 Jul 2020 14:19 +0200:
> +++ b/Src/zsh.h
> @@ -1205,9 +1207,15 @@ struct hashtable {
> -#ifdef HASHTABLE_INTERNAL_MEMBERS
> -    HASHTABLE_INTERNAL_MEMBERS	/* internal use in hashtable.c                */
> -#endif
> +    /* HASHTABLE INTERNAL MEMBERS */
> +    ScanStatus scan;		/* status of a scan over this hashtable       */
> +
> +#ifdef ZSH_HASH_DEBUG
> +    /* Members of struct hashtable used for debugging hash tables           */ \
> +    HashTable next, last;	/* linked list of all hash tables           */ \
> +    char *tablename;		/* string containing name of the hash table */ \
> +    PrintTableStats printinfo;	/* pointer to function to print table stats */
> +#endif /* !ZSH_HASH_DEBUG */
>  };

Thanks for looking into this.

It's clearly correct, but as written, the patch loses the distinction
that these members are private to hashtable.c and should not be accessed
by other parts of the code.  Could you address that, please?  If
there's an easy way to have the compiler enforce this restriction,
great; else, we can at least add a comment.

Cheers,

Daniel

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: 5.8: LTO exposes some new issues
  2020-07-27 14:19                   ` Roman Perepelitsa
@ 2020-07-28  8:09                     ` Daniel Shahaf
  2020-07-28 10:55                     ` Fwd: " Roman Perepelitsa
  1 sibling, 0 replies; 20+ messages in thread
From: Daniel Shahaf @ 2020-07-28  8:09 UTC (permalink / raw)
  To: Roman Perepelitsa; +Cc: Tomasz Kłoczko, Zsh hackers list

Roman Perepelitsa wrote on Mon, 27 Jul 2020 16:19 +0200:
> FYI: I won't be doing anything about the warning in gettempname (which
> I'm not getting with my toolchain).

I'm not sure there's anything we can do about that warning.  When I last
looked, the issue was that:

- The linker emits the warning whenever mktemp(3) is used;

- At least one callsite uses mktemp(3) unavoidably: it follows the call
  not by open(2) but by mknod(2), so the warning's rationale (that
  mkstemp(3) should be used in lieu of mktemp(3)) does not apply;

- The linker gives no way to disable the warning.

Unless there's an alternative to mktemp(3)+mknod(2) that I'm unaware
of, I'd triage this as a linker bug, in that it should be possible to
silence the warning when it's a false positive.

Cheers,

Daniel

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: 5.8: LTO exposes some new issues
  2020-07-27 12:46                 ` Tomasz Kłoczko
  2020-07-27 14:13                   ` Roman Perepelitsa
  2020-07-27 14:19                   ` Roman Perepelitsa
@ 2020-07-28  8:19                   ` Daniel Shahaf
  2 siblings, 0 replies; 20+ messages in thread
From: Daniel Shahaf @ 2020-07-28  8:19 UTC (permalink / raw)
  To: Tomasz Kłoczko; +Cc: Roman Perepelitsa, Zsh hackers list

Tomasz Kłoczko wrote on Mon, 27 Jul 2020 13:46 +0100:
> Other topic: do you have any plans to move to pcre2? :P

This wasn't discussed before.  Feel free to start a new thread
proposing it.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: 5.8: LTO exposes some new issues
  2020-07-28  7:53                 ` Daniel Shahaf
@ 2020-07-28  8:25                   ` Peter Stephenson
  2020-07-28 10:52                     ` Roman Perepelitsa
  0 siblings, 1 reply; 20+ messages in thread
From: Peter Stephenson @ 2020-07-28  8:25 UTC (permalink / raw)
  To: Zsh hackers list

> On 28 July 2020 at 08:53 Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> Roman Perepelitsa wrote on Mon, 27 Jul 2020 14:19 +0200:
> > +++ b/Src/zsh.h
> > @@ -1205,9 +1207,15 @@ struct hashtable {
> > -#ifdef HASHTABLE_INTERNAL_MEMBERS
> > -    HASHTABLE_INTERNAL_MEMBERS	/* internal use in hashtable.c                */
> > -#endif
> > +    /* HASHTABLE INTERNAL MEMBERS */
> > +    ScanStatus scan;		/* status of a scan over this hashtable       */
> > +
> > +#ifdef ZSH_HASH_DEBUG
> > +    /* Members of struct hashtable used for debugging hash tables           */ \
> > +    HashTable next, last;	/* linked list of all hash tables           */ \
> > +    char *tablename;		/* string containing name of the hash table */ \
> > +    PrintTableStats printinfo;	/* pointer to function to print table stats */
> > +#endif /* !ZSH_HASH_DEBUG */
> >  };
> 
> Thanks for looking into this.
> 
> It's clearly correct, but as written, the patch loses the distinction
> that these members are private to hashtable.c and should not be accessed
> by other parts of the code.  Could you address that, please?  If
> there's an easy way to have the compiler enforce this restriction,
> great; else, we can at least add a comment.

One way is to have a "struct { ... } private" substructure,
which it makes it clear what's going on within the code (though comments
are obviously useful, too).

pws

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: 5.8: LTO exposes some new issues
  2020-07-28  8:25                   ` Peter Stephenson
@ 2020-07-28 10:52                     ` Roman Perepelitsa
  2020-07-28 11:19                       ` Daniel Shahaf
  0 siblings, 1 reply; 20+ messages in thread
From: Roman Perepelitsa @ 2020-07-28 10:52 UTC (permalink / raw)
  To: Peter Stephenson; +Cc: Zsh hackers list

[-- Attachment #1: Type: text/plain, Size: 1240 bytes --]

On Tue, Jul 28, 2020 at 10:26 AM Peter Stephenson
<p.w.stephenson@ntlworld.com> wrote:
>
> > On 28 July 2020 at 08:53 Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> >
> > It's clearly correct, but as written, the patch loses the distinction
> > that these members are private to hashtable.c and should not be accessed
> > by other parts of the code.  Could you address that, please?  If
> > there's an easy way to have the compiler enforce this restriction,
> > great; else, we can at least add a comment.
>
> One way is to have a "struct { ... } private" substructure,
> which it makes it clear what's going on within the code (though comments
> are obviously useful, too).

How about this? The diff is a bit larger but the code is fairly
straightforward. Only hashtable.c has access to internal fields, just
like before the patch.

In a nutshell, struct hashtable has only public data members. Within
hashtable.c there is struct hashtableimpl, which has struct hashtable
as the first data member. C allows casting a pointer to a struct to a
pointer to its first data member and back without violating aliasing
rules. Thus hashtable.c can cast struct hashtable* to struct
hashtableimpl* in order to get access to internal fields.

Roman.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 7619 bytes --]

diff --git a/Src/hashtable.c b/Src/hashtable.c
index e210ddeca..e49f4e1da 100644
--- a/Src/hashtable.c
+++ b/Src/hashtable.c
@@ -28,25 +28,27 @@
  */
 
 #include "../config.h"
+#include "zsh.mdh"
+#include "hashtable.pro"
+
+typedef struct scanstatus *ScanStatus;
+typedef struct hashtableimpl* HashTableImpl;
+
+struct hashtableimpl {
+    struct hashtable pub;
+
+    /* HASHTABLE INTERNAL MEMBERS */
+    ScanStatus scan;		/* status of a scan over this hashtable     */
 
 #ifdef ZSH_HASH_DEBUG
-# define HASHTABLE_DEBUG_MEMBERS \
-    /* Members of struct hashtable used for debugging hash tables */ \
-    HashTable next, last;	/* linked list of all hash tables           */ \
-    char *tablename;		/* string containing name of the hash table */ \
+    /* HASHTABLE DEBUG MEMBERS */
+    HashTableImpl next, last;	/* linked list of all hash tables           */
+    char *tablename;		/* string containing name of the hash table */
     PrintTableStats printinfo;	/* pointer to function to print table stats */
-#else /* !ZSH_HASH_DEBUG */
-# define HASHTABLE_DEBUG_MEMBERS
 #endif /* !ZSH_HASH_DEBUG */
+};
 
-#define HASHTABLE_INTERNAL_MEMBERS \
-    ScanStatus scan;		/* status of a scan over this hashtable     */ \
-    HASHTABLE_DEBUG_MEMBERS
-
-typedef struct scanstatus *ScanStatus;
-
-#include "zsh.mdh"
-#include "hashtable.pro"
+static inline HashTableImpl impl(HashTable ht) { return (HashTableImpl)ht; }
 
 /* Structure for recording status of a hashtable scan in progress.  When a *
  * scan starts, the .scan member of the hashtable structure points to one  *
@@ -71,7 +73,8 @@ struct scanstatus {
 /********************************/
 
 #ifdef ZSH_HASH_DEBUG
-static HashTable firstht, lastht;
+static void printhashtabinfo(HashTable ht);
+static HashTableImpl firstht, lastht;
 #endif /* ZSH_HASH_DEBUG */
 
 /* Generic hash function */
@@ -94,9 +97,9 @@ hasher(const char *str)
 mod_export HashTable
 newhashtable(int size, UNUSED(char const *name), UNUSED(PrintTableStats printinfo))
 {
-    HashTable ht;
+    HashTableImpl ht;
 
-    ht = (HashTable) zshcalloc(sizeof *ht);
+    ht = (HashTableImpl) zshcalloc(sizeof *ht);
 #ifdef ZSH_HASH_DEBUG
     ht->next = NULL;
     if(!firstht)
@@ -108,12 +111,12 @@ newhashtable(int size, UNUSED(char const *name), UNUSED(PrintTableStats printinf
     ht->printinfo = printinfo ? printinfo : printhashtabinfo;
     ht->tablename = ztrdup(name);
 #endif /* ZSH_HASH_DEBUG */
-    ht->nodes = (HashNode *) zshcalloc(size * sizeof(HashNode));
-    ht->hsize = size;
-    ht->ct = 0;
+    ht->pub.nodes = (HashNode *) zshcalloc(size * sizeof(HashNode));
+    ht->pub.hsize = size;
+    ht->pub.ct = 0;
     ht->scan = NULL;
-    ht->scantab = NULL;
-    return ht;
+    ht->pub.scantab = NULL;
+    return &ht->pub;
 }
 
 /* Delete a hash table.  After this function has been used, any *
@@ -125,18 +128,18 @@ deletehashtable(HashTable ht)
 {
     ht->emptytable(ht);
 #ifdef ZSH_HASH_DEBUG
-    if(ht->next)
-	ht->next->last = ht->last;
+    if(impl(ht)->next)
+	impl(ht)->next->last = impl(ht)->last;
     else
-	lastht = ht->last;
-    if(ht->last)
-	ht->last->next = ht->next;
+	lastht = impl(ht)->last;
+    if(impl(ht)->last)
+	impl(ht)->last->next = impl(ht)->next;
     else
-	firstht = ht->next;
-    zsfree(ht->tablename);
+	firstht = impl(ht)->next;
+    zsfree(impl(ht)->tablename);
 #endif /* ZSH_HASH_DEBUG */
     zfree(ht->nodes, ht->hsize * sizeof(HashNode));
-    zfree(ht, sizeof(*ht));
+    zfree(ht, sizeof(struct hashtableimpl));
 }
 
 /* Add a node to a hash table.                          *
@@ -175,7 +178,7 @@ addhashnode2(HashTable ht, char *nam, void *nodeptr)
     if (!hp) {
 	hn->next = NULL;
 	ht->nodes[hashval] = hn;
-	if (++ht->ct >= ht->hsize * 2 && !ht->scan)
+	if (++ht->ct >= ht->hsize * 2 && !impl(ht)->scan)
 	    expandhashtable(ht);
 	return NULL;
     }
@@ -185,15 +188,15 @@ addhashnode2(HashTable ht, char *nam, void *nodeptr)
 	ht->nodes[hashval] = hn;
 	replacing:
 	hn->next = hp->next;
-	if(ht->scan) {
-	    if(ht->scan->sorted) {
-		HashNode *hashtab = ht->scan->u.s.hashtab;
+	if(impl(ht)->scan) {
+	    if(impl(ht)->scan->sorted) {
+		HashNode *hashtab = impl(ht)->scan->u.s.hashtab;
 		int i;
-		for(i = ht->scan->u.s.ct; i--; )
+		for(i = impl(ht)->scan->u.s.ct; i--; )
 		    if(hashtab[i] == hp)
 			hashtab[i] = hn;
-	    } else if(ht->scan->u.u == hp)
-		ht->scan->u.u = hn;
+	    } else if(impl(ht)->scan->u.u == hp)
+		impl(ht)->scan->u.u = hn;
 	}
 	return hp;
     }
@@ -211,7 +214,7 @@ addhashnode2(HashTable ht, char *nam, void *nodeptr)
     /* else just add it at the front of the list */
     hn->next = ht->nodes[hashval];
     ht->nodes[hashval] = hn;
-    if (++ht->ct >= ht->hsize * 2 && !ht->scan)
+    if (++ht->ct >= ht->hsize * 2 && !impl(ht)->scan)
         expandhashtable(ht);
     return NULL;
 }
@@ -284,15 +287,15 @@ removehashnode(HashTable ht, const char *nam)
 	ht->nodes[hashval] = hp->next;
 	gotit:
 	ht->ct--;
-	if(ht->scan) {
-	    if(ht->scan->sorted) {
-		HashNode *hashtab = ht->scan->u.s.hashtab;
+	if(impl(ht)->scan) {
+	    if(impl(ht)->scan->sorted) {
+		HashNode *hashtab = impl(ht)->scan->u.s.hashtab;
 		int i;
-		for(i = ht->scan->u.s.ct; i--; )
+		for(i = impl(ht)->scan->u.s.ct; i--; )
 		    if(hashtab[i] == hp)
 			hashtab[i] = NULL;
-	    } else if(ht->scan->u.u == hp)
-		ht->scan->u.u = hp->next;
+	    } else if(impl(ht)->scan->u.u == hp)
+		impl(ht)->scan->u.u = hp->next;
 	}
 	return hp;
     }
@@ -399,7 +402,7 @@ scanmatchtable(HashTable ht, Patprog pprog, int sorted,
 	st.sorted = 1;
 	st.u.s.hashtab = hnsorttab;
 	st.u.s.ct = ct;
-	ht->scan = &st;
+	impl(ht)->scan = &st;
 
 	for (htp = hnsorttab, i = 0; i < ct; i++, htp++) {
 	    if ((!flags1 || ((*htp)->flags & flags1)) &&
@@ -410,13 +413,13 @@ scanmatchtable(HashTable ht, Patprog pprog, int sorted,
 	    }
 	}
 
-	ht->scan = NULL;
+	impl(ht)->scan = NULL;
     } else {
 	int i, hsize = ht->hsize;
 	HashNode *nodes = ht->nodes;
 
 	st.sorted = 0;
-	ht->scan = &st;
+	impl(ht)->scan = &st;
 
 	for (i = 0; i < hsize; i++)
 	    for (st.u.u = nodes[i]; st.u.u; ) {
@@ -429,7 +432,7 @@ scanmatchtable(HashTable ht, Patprog pprog, int sorted,
 		}
 	    }
 
-	ht->scan = NULL;
+	impl(ht)->scan = NULL;
     }
 
     return match;
@@ -531,7 +534,7 @@ printhashtabinfo(HashTable ht)
     int chainlen[MAXDEPTH + 1];
     int i, tmpcount, total;
 
-    printf("name of table   : %s\n",   ht->tablename);
+    printf("name of table   : %s\n",   impl(ht)->tablename);
     printf("size of nodes[] : %d\n",   ht->hsize);
     printf("number of nodes : %d\n\n", ht->ct);
 
@@ -560,12 +563,12 @@ printhashtabinfo(HashTable ht)
 int
 bin_hashinfo(UNUSED(char *nam), UNUSED(char **args), UNUSED(Options ops), UNUSED(int func))
 {
-    HashTable ht;
+    HashTableImpl ht;
 
     printf("----------------------------------------------------\n");
     queue_signals();
     for(ht = firstht; ht; ht = ht->next) {
-	ht->printinfo(ht);
+	ht->printinfo(&ht->pub);
 	printf("----------------------------------------------------\n");
     }
     unqueue_signals();
diff --git a/Src/zsh.h b/Src/zsh.h
index c48be4ffd..6101cf242 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -1204,10 +1204,6 @@ struct hashtable {
     FreeNodeFunc freenode;	/* pointer to function to free a node         */
     ScanFunc printnode;		/* pointer to function to print a node        */
     ScanTabFunc scantab;	/* pointer to function to scan table          */
-
-#ifdef HASHTABLE_INTERNAL_MEMBERS
-    HASHTABLE_INTERNAL_MEMBERS	/* internal use in hashtable.c                */
-#endif
 };
 
 /* generic hash table node */

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: Fwd: 5.8: LTO exposes some new issues
  2020-07-27 14:19                   ` Roman Perepelitsa
  2020-07-28  8:09                     ` Daniel Shahaf
@ 2020-07-28 10:55                     ` Roman Perepelitsa
  1 sibling, 0 replies; 20+ messages in thread
From: Roman Perepelitsa @ 2020-07-28 10:55 UTC (permalink / raw)
  To: Tomasz Kłoczko; +Cc: Daniel Shahaf, Zsh hackers list

On Mon, Jul 27, 2020 at 4:19 PM Roman Perepelitsa
<roman.perepelitsa@gmail.com> wrote:
>
> On Mon, Jul 27, 2020 at 2:47 PM Tomasz Kłoczko <kloczko.tomasz@gmail.com> wrote:
> > Here is whole list of compile and linking warnings:
> >
> > utils.c: In function 'getkeystring':
> > lto1: warning: function may return address of local variable [-Wreturn-local-addr]
> > utils.c:6644:16: note: declared here
> >  6644 |     char *buf, tmp[1];
> >       |                ^
>
> This one might be a real bug. [...]
> I'm attaching a patch [...]

kloczko.tomasz@gmail.com has informed me privately that this patch
doesn't silence the warning. I won't be applying it or pursuing this
matter any further.

Roman.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: 5.8: LTO exposes some new issues
  2020-07-28 10:52                     ` Roman Perepelitsa
@ 2020-07-28 11:19                       ` Daniel Shahaf
  2020-07-28 11:31                         ` Roman Perepelitsa
  0 siblings, 1 reply; 20+ messages in thread
From: Daniel Shahaf @ 2020-07-28 11:19 UTC (permalink / raw)
  To: Roman Perepelitsa; +Cc: Peter Stephenson, Zsh hackers list

Roman Perepelitsa wrote on Tue, 28 Jul 2020 12:52 +0200:
> On Tue, Jul 28, 2020 at 10:26 AM Peter Stephenson
> <p.w.stephenson@ntlworld.com> wrote:
> >  
> > > On 28 July 2020 at 08:53 Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > >
> > > It's clearly correct, but as written, the patch loses the distinction
> > > that these members are private to hashtable.c and should not be accessed
> > > by other parts of the code.  Could you address that, please?  If
> > > there's an easy way to have the compiler enforce this restriction,
> > > great; else, we can at least add a comment.  
> >
> > One way is to have a "struct { ... } private" substructure,
> > which it makes it clear what's going on within the code (though comments
> > are obviously useful, too).  
> 
> How about this? The diff is a bit larger but the code is fairly
> straightforward. Only hashtable.c has access to internal fields, just
> like before the patch.
> 
> In a nutshell, struct hashtable has only public data members. Within
> hashtable.c there is struct hashtableimpl, which has struct hashtable
> as the first data member. C allows casting a pointer to a struct to a
> pointer to its first data member and back without violating aliasing
> rules. Thus hashtable.c can cast struct hashtable* to struct
> hashtableimpl* in order to get access to internal fields.

Thanks, that addresses the previous point, but unfortunately it creates
another problem: people who read the .h file are liable to declare
local variables of type 'struct hashtable', or memcpy() them around,
and in either case, once such a variable gets to hashtable.c and the
private members are accessed, we'll get out-of-bounds reads.

So we need either a comment at the definition of the struct type that
says nobody should allocate/duplicate/assign such structs directly, but
call newhashtable() instead, or a solution that doesn't involve casts,
such as Peter's proposal.

Cheers,

Daniel

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: 5.8: LTO exposes some new issues
  2020-07-28 11:19                       ` Daniel Shahaf
@ 2020-07-28 11:31                         ` Roman Perepelitsa
  2020-07-28 11:51                           ` Daniel Shahaf
  0 siblings, 1 reply; 20+ messages in thread
From: Roman Perepelitsa @ 2020-07-28 11:31 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: Peter Stephenson, Zsh hackers list

On Tue, Jul 28, 2020 at 1:20 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>
> Roman Perepelitsa wrote on Tue, 28 Jul 2020 12:52 +0200:
> >
> > How about this? The diff is a bit larger but the code is fairly
> > straightforward. Only hashtable.c has access to internal fields, just
> > like before the patch.
> >
> > In a nutshell, struct hashtable has only public data members. Within
> > hashtable.c there is struct hashtableimpl, which has struct hashtable
> > as the first data member. C allows casting a pointer to a struct to a
> > pointer to its first data member and back without violating aliasing
> > rules. Thus hashtable.c can cast struct hashtable* to struct
> > hashtableimpl* in order to get access to internal fields.
>
> Thanks, that addresses the previous point, but unfortunately it creates
> another problem: people who read the .h file are liable to declare
> local variables of type 'struct hashtable', or memcpy() them around,
> and in either case, once such a variable gets to hashtable.c and the
> private members are accessed, we'll get out-of-bounds reads.

This problem exists in the current version of the code, too. The patch
addresses one problem -- it removes undefined behavior due to ODR
violation. If you want, I can extend the patch so that it also
addresses the second problem you've identified although it might be
betted done in a separate patch given that it's independent from the
first.

Roman.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: 5.8: LTO exposes some new issues
  2020-07-28 11:31                         ` Roman Perepelitsa
@ 2020-07-28 11:51                           ` Daniel Shahaf
  0 siblings, 0 replies; 20+ messages in thread
From: Daniel Shahaf @ 2020-07-28 11:51 UTC (permalink / raw)
  To: Roman Perepelitsa; +Cc: Peter Stephenson, Zsh hackers list

Roman Perepelitsa wrote on Tue, 28 Jul 2020 13:31 +0200:
> On Tue, Jul 28, 2020 at 1:20 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> >
> > Roman Perepelitsa wrote on Tue, 28 Jul 2020 12:52 +0200:  
> > >
> > > How about this? The diff is a bit larger but the code is fairly
> > > straightforward. Only hashtable.c has access to internal fields, just
> > > like before the patch.
> > >
> > > In a nutshell, struct hashtable has only public data members. Within
> > > hashtable.c there is struct hashtableimpl, which has struct hashtable
> > > as the first data member. C allows casting a pointer to a struct to a
> > > pointer to its first data member and back without violating aliasing
> > > rules. Thus hashtable.c can cast struct hashtable* to struct
> > > hashtableimpl* in order to get access to internal fields.  
> >
> > Thanks, that addresses the previous point, but unfortunately it creates
> > another problem: people who read the .h file are liable to declare
> > local variables of type 'struct hashtable', or memcpy() them around,
> > and in either case, once such a variable gets to hashtable.c and the
> > private members are accessed, we'll get out-of-bounds reads.  
> 
> This problem exists in the current version of the code, too. The patch
> addresses one problem -- it removes undefined behavior due to ODR
> violation. If you want, I can extend the patch so that it also
> addresses the second problem you've identified although it might be
> betted done in a separate patch given that it's independent from the
> first.

Whatever you think best.  In general, if in doubt I'd err on the side
of splitting.  Thanks again for looking into this. ☺

Daniel

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2020-07-28 11:51 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-21 23:41 5.8: LTO exposes some new issues Tomasz Kłoczko
2020-07-22  5:59 ` Daniel Shahaf
2020-07-25 17:43   ` Bart Schaefer
     [not found]     ` <CABB28CxSD5w-SY-iCVYuQ4kJfBpNJOWhpk4HOrS1DNPfMVztgw@mail.gmail.com>
2020-07-25 20:05       ` Fwd: " Bart Schaefer
2020-07-27  2:12         ` Daniel Shahaf
2020-07-27 10:07           ` Tomasz Kłoczko
2020-07-27 11:09             ` Roman Perepelitsa
2020-07-27 12:19               ` Roman Perepelitsa
2020-07-27 12:46                 ` Tomasz Kłoczko
2020-07-27 14:13                   ` Roman Perepelitsa
2020-07-27 14:19                   ` Roman Perepelitsa
2020-07-28  8:09                     ` Daniel Shahaf
2020-07-28 10:55                     ` Fwd: " Roman Perepelitsa
2020-07-28  8:19                   ` Daniel Shahaf
2020-07-28  7:53                 ` Daniel Shahaf
2020-07-28  8:25                   ` Peter Stephenson
2020-07-28 10:52                     ` Roman Perepelitsa
2020-07-28 11:19                       ` Daniel Shahaf
2020-07-28 11:31                         ` Roman Perepelitsa
2020-07-28 11:51                           ` Daniel Shahaf

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/zsh/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).