[-- 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>*
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.
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.
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
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
>
[-- 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
[-- 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 */
[-- 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 */
[-- 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
[-- 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 */
[-- 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;
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
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
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.
> 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
[-- 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 */
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.
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
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.
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