Re: CVS commit: src/sys
On 03.02.2021 16:52, Roy Marples wrote: > But you *have* to interogate the headers in order to work this out. I noted how this could break. I'm right now not affected by this myself. Please monitor gnats for reports. I defer any decisions and discussions (if any) now to others.
Re: CVS commit: src/sys
On 03.02.2021 13:39, Roy Marples wrote:
> On 03/02/2021 10:03, Kamil Rytarowski wrote:
>> On 03.02.2021 06:51, Roy Marples wrote:
>>> Module Name: src
>>> Committed By: roy
>>> Date: Wed Feb 3 05:51:40 UTC 2021
>>>
>>> Modified Files:
>>> src/sys/net: if_arp.h if_ether.h if_gre.h
>>> src/sys/netinet: if_ether.h igmp.h in.h ip.h ip6.h ip_carp.h
>>> ip_icmp.h
>>> ip_mroute.h ip_var.h tcp.h tcp_debug.h tcp_var.h udp.h udp_var.h
>>>
>>> Log Message:
>>> Remove __packed from various network structures
>>>
>>> They are already network aligned and adding the __packed attribute
>>> just causes needless compiler warnings about accssing members of packed
>>> objects.
>>>
>>
>> This changes the ABI for userland programs. With __packed, these
>> structures, whenever contain at least 2-byte data, can be placed in a
>> different location inside another structure now.
>>
>> #include
>> #include
>> #include
>> #include
>>
>> struct aaa {
>> uint16_t a;
>> uint8_t b;
>> uint8_t c;
>> } __packed;
>>
>> struct aaa2 {
>> uint8_t x;
>> struct aaa y;
>> };
>>
>> struct bbb {
>> uint16_t a;
>> uint8_t b;
>> uint8_t c;
>> };
>>
>> struct bbb2 {
>> uint8_t x;
>> struct bbb y;
>> };
>
> Assuming that struct aaa and bbb are from NetBSD headers and aaa2 and
> bbb2 are your own constructs then you just have yourself to blame.
>
It is a valid code to contain packed data in a structure without the
packed attribute.
> struct bbb2 {
> uint8_t x;
> struct bbb y;
> } __packed;
>
> Makes bbb2 the same as aaa2.
>
>> Before I saw your commit, I wanted to ask to revert the following
>> changes:
>>
>> icmp6: Remove __packed attribute from icmp6 structures
>>
>> https://github.com/NetBSD/src/commit/427831ba4bdf388aecf3a378de8faf3a4d44a462
>>
>>
>> ip6: Remove __packed attribute from ip6 structures
>>
>> https://github.com/NetBSD/src/commit/e82879afd70b0e801e6ee53bd14c27be3dd1738f
>>
>>
>> The fallout can be subtle and hard to debug. Once, I removed __packed
>> from one innocent networking structure myself, qemu networking stopped
>> working at all.
>
> How you use the structure is up to you.
> For the record, we were the only BSD to ever apply __packed to these
> structures and thanks to modern compilers emitting these wonderful
> warnings it proved to be a bad move.
>
This is still a valid usage and ABI breakage for userland. You cannot
blame a user for using system structures and headers that stop working
after an upgrade, at least before at least libc version bump.
For the record, I broke ABI here (it was the reverse situation, addition
of __packed).
https://github.com/NetBSD/src/commit/a833bd5cfdba983ecb5560512a3547f46f35f11e
I vote to revert and handling these structures with appropriate
functions that are aware of potentially misaligned data operations.
If we you or the project resist and insists on ABI breakage, it should
be boldly documented.
> Roy
Re: CVS commit: src/sys
On 03.02.2021 06:51, Roy Marples wrote:
> Module Name: src
> Committed By: roy
> Date: Wed Feb 3 05:51:40 UTC 2021
>
> Modified Files:
> src/sys/net: if_arp.h if_ether.h if_gre.h
> src/sys/netinet: if_ether.h igmp.h in.h ip.h ip6.h ip_carp.h ip_icmp.h
> ip_mroute.h ip_var.h tcp.h tcp_debug.h tcp_var.h udp.h udp_var.h
>
> Log Message:
> Remove __packed from various network structures
>
> They are already network aligned and adding the __packed attribute
> just causes needless compiler warnings about accssing members of packed
> objects.
>
This changes the ABI for userland programs. With __packed, these
structures, whenever contain at least 2-byte data, can be placed in a
different location inside another structure now.
#include
#include
#include
#include
struct aaa {
uint16_t a;
uint8_t b;
uint8_t c;
} __packed;
struct aaa2 {
uint8_t x;
struct aaa y;
};
struct bbb {
uint16_t a;
uint8_t b;
uint8_t c;
};
struct bbb2 {
uint8_t x;
struct bbb y;
};
int
main()
{
printf("sizeof(aaa2) = %zu, alignof(aaa2) = %zu\n",
sizeof(struct aaa2), alignof(struct aaa2));
printf("sizeof(bbb2) = %zu, alignof(bbb2) = %zu\n",
sizeof(struct bbb2), alignof(struct bbb2));
}
$ ./a.out
sizeof(aaa) = 4, alignof(aaa) = 1
sizeof(bbb) = 4, alignof(bbb) = 2
sizeof(aaa2) = 5, alignof(aaa2) = 1
sizeof(bbb2) = 6, alignof(bbb2) = 2
Please try t access these members of the structs with memcpy(3),
memcmp(3), memset(3) etc rather than directly with * or [].
Before I saw your commit, I wanted to ask to revert the following changes:
icmp6: Remove __packed attribute from icmp6 structures
https://github.com/NetBSD/src/commit/427831ba4bdf388aecf3a378de8faf3a4d44a462
ip6: Remove __packed attribute from ip6 structures
https://github.com/NetBSD/src/commit/e82879afd70b0e801e6ee53bd14c27be3dd1738f
The fallout can be subtle and hard to debug. Once, I removed __packed
from one innocent networking structure myself, qemu networking stopped
working at all.
Re: CVS commit: src/lib/libc/stdio
On 31.01.2021 18:34, Joerg Sonnenberger wrote: > On Sun, Jan 31, 2021 at 05:27:28PM +0100, Kamil Rytarowski wrote: >> On 31.01.2021 17:18, Jaromir Dolecek wrote: >>> Module Name:src >>> Committed By: jdolecek >>> Date: Sun Jan 31 16:18:22 UTC 2021 >>> >>> Modified Files: >>> src/lib/libc/stdio: fread.c >>> >>> Log Message: >>> for unbuffered I/O arrange for the destination buffer to be filled in one >>> go, instead of triggering long series of 1 byte read(2)s; this speeds up >>> fread() several order of magnitudes for this case, directly proportional >>> to the size of the supplied buffer >>> >>> change adapted from OpenBSD rev. 1.19 >>> >>> fixes PR lib/55808 by Roland Illig >>> >> >> While there it would be cool to apply FreeBSD and OpenBSD hardening for >> invalid size x nmemb, checking for overflow. I propose to return EINVAL >> in such case. > > That makes no sense. Just turn them into a short read, which is > something users have to deal with anyway. > > Joerg > The purpose of this errno is to catch insage usage of API that in normal circumstances never makes sense. With overflows (that are fine), the error can be unnoticed. Both FreeBSD and OpenBSD found this behavior as useful.
Re: CVS commit: src/lib/libc/stdio
On 31.01.2021 17:18, Jaromir Dolecek wrote: > Module Name: src > Committed By: jdolecek > Date: Sun Jan 31 16:18:22 UTC 2021 > > Modified Files: > src/lib/libc/stdio: fread.c > > Log Message: > for unbuffered I/O arrange for the destination buffer to be filled in one > go, instead of triggering long series of 1 byte read(2)s; this speeds up > fread() several order of magnitudes for this case, directly proportional > to the size of the supplied buffer > > change adapted from OpenBSD rev. 1.19 > > fixes PR lib/55808 by Roland Illig > While there it would be cool to apply FreeBSD and OpenBSD hardening for invalid size x nmemb, checking for overflow. I propose to return EINVAL in such case. I planned to submit it a while ago for discussion.
Re: CVS commit: src/sys/arch
On 25.01.2021 17:19, Jason Thorpe wrote:
>
>> On Jan 25, 2021, at 6:22 AM, Kamil Rytarowski wrote:
>>
>> I have no problem with this change but I am curious why should we use "{
>> }"? It's a C GNU extension and C++ syntax.
>
> Using { 0 } makes an assumption about the first member of the structure which
> is not guaranteed to remain true.
>
Both versions should work in the same way, except that {0} is the
standard variation and {} an extension.
I have got no preference for either.
> -- thorpej
>
signature.asc
Description: OpenPGP digital signature
Re: CVS commit: src/sys/arch
On 25.01.2021 15:20, Jason R Thorpe wrote:
> Module Name: src
> Committed By: thorpej
> Date: Mon Jan 25 14:20:39 UTC 2021
>
> Modified Files:
> src/sys/arch/arm/altera: cycv_clkmgr.c
> src/sys/arch/arm/amlogic: meson_pinctrl.c meson_pwm.c meson_thermal.c
> meson_usbctrl.c meson_usbphy.c mesong12_clkc.c mesongx_mmc.c
> mesongxbb_clkc.c
> src/sys/arch/arm/broadcom: bcm2835_emmc.c bcm2835_intr.c
> src/sys/arch/arm/fdt: gicv3_fdt.c pcihost_fdt.c
> src/sys/arch/arm/nvidia: tegra_ahcisata.c tegra_nouveau.c tegra_pcie.c
> tegra_pinmux.c tegra_soctherm.c tegra_xusb.c
> src/sys/arch/arm/nxp: if_enet_imx.c imx6_pcie.c imx6_spi.c
> imx8mq_usbphy.c imx_sdhc.c
> src/sys/arch/arm/rockchip: rk3328_iomux.c rk3399_iomux.c rk_gmac.c
> rk_i2s.c rk_pwm.c rk_tsadc.c rk_usb.c rk_v1crypto.c rk_vop.c
> src/sys/arch/arm/samsung: exynos_dwcmmc.c exynos_pinctrl.c
> exynos_platform.c exynos_usbdrdphy.c exynos_usbphy.c
> src/sys/arch/arm/sociox: if_ave.c
> src/sys/arch/arm/sunxi: sun4i_a10_ccu.c sun4i_dma.c sun6i_dma.c
> sun8i_crypto.c sunxi_can.c sunxi_codec.c sunxi_de2_ccu.c
> sunxi_dep.c sunxi_gpio.c sunxi_hdmi.c sunxi_hdmiphy.c sunxi_i2s.c
> sunxi_lcdc.c sunxi_lradc.c sunxi_mixer.c sunxi_mmc.c sunxi_musb.c
> sunxi_nmi.c sunxi_pwm.c sunxi_rsb.c sunxi_rtc.c sunxi_sid.c
> sunxi_sramc.c sunxi_tcon.c sunxi_thermal.c sunxi_ts.c sunxi_twi.c
> sunxi_usb3phy.c sunxi_usbphy.c sunxi_wdt.c
> src/sys/arch/arm/ti: ti_dpll_clock.c ti_gpio.c ti_iic.c ti_omapintc.c
> ti_omaptimer.c ti_sdhc.c
> src/sys/arch/macppc/dev: deq.c lmu.c psoc.c smusat.c
> src/sys/arch/mips/cavium/dev: octeon_cib.c octeon_intc.c
> src/sys/arch/sparc64/dev: pcf8591_envctrl.c
>
> Log Message:
> Since we're using designated initialisers for compat data, we should
> use a completely empty initializer for the sentinel.
>
> static const struct device_compatible_entry compat_data[] = {
> { .compat = "ecadc" },
> -
> - { 0 }
> + { }
> };
>
> static int
>
I have no problem with this change but I am curious why should we use "{
}"? It's a C GNU extension and C++ syntax.
signature.asc
Description: OpenPGP digital signature
Re: CVS commit: src/external/gpl3/gdb/dist/gdb
On 10.12.2020 08:14, Rin Okuyama wrote: > Module Name: src > Committed By: rin > Date: Thu Dec 10 07:14:58 UTC 2020 > > Modified Files: > src/external/gpl3/gdb/dist/gdb: nbsd-nat.c > > Log Message: > Fix arm, for which PT_STEP is defined but unimplemented. > > XXX > Stop exposing PT_STEP to userland for arm? > Yes, please remove it from headers for arm. This was a hack for DTrace as far as I recall, to expose PT_STEP that way and get DTrace building. Then, please restore nbsd-nat.c to the previous state. signature.asc Description: OpenPGP digital signature
Re: please put back cat man pages, and what's the deal with warp?
On 13.11.2020 09:19, nia wrote: > On Wed, Nov 11, 2020 at 01:12:58PM +0100, Kamil Rytarowski wrote: >> On 11.11.2020 11:05, nia wrote: >>> https://github.com/NetBSD/src/blob/8dacd60b50e3047dcbb2645d41df02cd16fd2072/games/warp/warp.c#L10 >>> >>> this doesn't read like a netbsd-compatible software license. >>> >> >> Fixed. It was an leftover. If there is anything else, please let me know. > > Did you actually get permission from him to donate copyright to TNF or > are you just interpreting his vague statement mentioned in your commit as > "do whatever you like"? That's not how copyright works. > The commit contains Larry's signoff. If you have some concerns please check in private (with e.g. Larry) and stop insinuating in public.
Re: please put back cat man pages, and what's the deal with warp?
On 11.11.2020 11:05, nia wrote: > https://github.com/NetBSD/src/blob/8dacd60b50e3047dcbb2645d41df02cd16fd2072/games/warp/warp.c#L10 > > this doesn't read like a netbsd-compatible software license. > Fixed. It was an leftover. If there is anything else, please let me know. > it's using libcompat. should new programs in base really be relying on > ancient bsd compat? > No, but on the other hand it was exposed that software using old tty is not buildable after this commit: https://github.com/NetBSD/src/commit/573acd495de4cf3c5dbe2c50ec5071a25f8027e6 Symbols such as ECHO are missing. Can we either drop ioctl_compat.h or reintroduce the symbols? Missing old and new tty headers didn't work when I tried it (at least before more renovation in warp(6) from christos@).
Re: please put back cat man pages, and what's the deal with warp?
On 10.11.2020 10:55, matthew green wrote: > at this point the right thing is to revert, and then proceed > with the discussion. if you can convince people that your > idea is of merit, you can put it back in, but right now that > is quite clearly not a shared sentiment. OK.
Re: please put back cat man pages, and what's the deal with warp?
On 10.11.2020 10:40, Robert Elz wrote: > Date:Tue, 10 Nov 2020 10:07:32 +0100 > From: Kamil Rytarowski > Message-ID: > > | Revert MKCATPAGES change? > > No, the changes to the mtree config that stopped creating the catN > directories, and anything else that has happened that is not 100% MKCATPAGES. > This was intended in the proposal and I was specifically asked by wiz@ (kind of our maintainer for man-pages) in a private mail to remember to purge these directories too. I was also the only user of these dirs myself as all other ones are presumed extinct. > On games/dungeon > > | but it needs rewrite (from Fortrant) and to avoid (c) issues. > > A rewrite form Fortran will not avoid copyright issues, that's > just a translation, and translations are covered. > That's way I wrote about a rewrite that avoids (c) issues, not translation or conversion f2c-like. > kre >
Re: please put back cat man pages, and what's the deal with warp?
On 10.11.2020 09:19, matthew green wrote: > there was not nearly enough discussion for this and i object > quite strongly about this. please revert immediately and > begin a real discussion. > Revert MKCATPAGES change? It was proposed 17 ago without objects and according to core@ rules for removals. I waited even more than a week before the final removal. > additionally, what's the deal with games/warp? i don't see > it discussed anywhere, and you've imported files that are > all executable. it's also pretty far-fetch to claim that > your commit was approved by lwall. huh? > I'm going to submit a dedicated mail! Restoration of BSD games was discussed and performed by perry@. Only warp and dungeon were still missing (and documented as so in games/Makefile). I have got the dungeon sources now too, but it needs rewrite (from Fortrant) and to avoid (c) issues. I was in touch with lwall@ over the past month on this topic. I will talk to an admin about the permissions. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src
On 08.11.2020 22:56, Nia Alarie wrote: > Module Name: src > Committed By: nia > Date: Sun Nov 8 21:56:48 UTC 2020 > > Modified Files: > src/external/bsd/kyua-cli: Makefile.inc > src/external/ibm-public/postfix: Makefile.inc > src/external/public-domain/sqlite: Makefile.inc > src/external/public-domain/sqlite/bin: Makefile > src/external/public-domain/sqlite/lib: Makefile sqlite3.pc.in > src/usr.sbin/makemandb: Makefile > > Log Message: > sqlite: do not build without multithreading support > > at least a few pkgsrc packages avoid base sqlite because it fails > this check, and it's probably a surprising performance penalty for > unsuspecting users > I'm getting: dependall ===> usr.sbin/makemandb nbmake[6]: nbmake[6]: don't know how to make -ltermlib. Stop nbmake[6]: stopped in /usr/src/usr.sbin/makemandb signature.asc Description: OpenPGP digital signature
Re: catman (Was: CVS commit: src/games/fortune/datfiles)
On 08.11.2020 23:20, Valery Ushakov wrote: > It's (partially) past-tensed, which looks stupid and cripples the > joke. catman has zero to do with current UNIX or any other standard I checked (SVID, XPG, POSIX, XNS, SUS, ISO, ANSI). It was a historical utility. I've changed it to be relatively accurate for a historical reference to Unix/BSD. I object to calling it a current Unix thing and suggesting to users that they can find it in section 8. If catman(8) is still delivered somewhere, it is documenting a dead toy. On 09.11.2020 01:05, Thomas Klausner wrote: > On Mon, Nov 09, 2020 at 12:46:42AM +0300, Valeriy E. Ushakov wrote: >> Also, come to think of it... Removing catman (i.e. user's ability to >> generate cat pages) is rather different from removing MKCATPAGES, >> what's going on here? > > I asked kamil about something else on this topic and he mentioned > catman. On reading the man pages I didn't really see a reason for it > to stay if we remove the cat pages completely. > > Do you see it differently? If yes, why? > Thomas > catman is still reachable for users (I still have no evidence that I was not the last user of .cat pages in NetBSD) that really want it in contrib/ of mandoc, but out of the distribution. I'm still test-building the final removal and I will land it once I feel comfortable about it. signature.asc Description: OpenPGP digital signature
Re: catman (Was: CVS commit: src/games/fortune/datfiles)
On 08.11.2020 22:46, Valery Ushakov wrote: > On Sun, Nov 08, 2020 at 17:37:30 +0000, Kamil Rytarowski wrote: > >> Module Name: src >> Committed By:kamil >> Date:Sun Nov 8 17:37:30 UTC 2020 >> >> Modified Files: >> src/games/fortune/datfiles: fortunes >> >> Log Message: >> catman(8) is a past thing > > Please, revert this. It's completely irrelevant for the joke if > netbsd ships catman or not. > > Also, come to think of it... Removing catman (i.e. user's ability to > generate cat pages) is rather different from removing MKCATPAGES, > what's going on here? > > -uwe > catman(8) is to removed soon as discussed with wiz@. The fortune is still there, it's not removed. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src
On 08.11.2020 17:55, Christos Zoulas wrote: > In article <20201108145236.3a009f...@cvs.netbsd.org>, > Kamil Rytarowski wrote: >> -=-=-=-=-=- >> >> Module Name: src >> Committed By:kamil >> Date:Sun Nov 8 14:52:36 UTC 2020 >> >> Modified Files: >> src: BUILDING >> src/distrib/sets: sets.subr >> src/doc: BUILDING.mdoc >> src/share/man/man5: mk.conf.5 >> src/share/mk: bsd.README bsd.man.mk bsd.own.mk >> >> Log Message: >> Remove the support for MKCATPAGES >> >> It was optional since 1999 and disabled by default since 2012. >> >> Proposed on tech-userlevel@. > > What about the sets? > > christos > I'm test-building a local patch removing the cat files, directories and a few other remnants. Once it will be done, I will land it. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/external/gpl3/gcc/dist/gcc/config/aarch64
On 15.10.2020 17:14, Rin Okuyama wrote: > On 2020/10/15 16:12, matthew green wrote: >> Martin Husemann writes: >>> On Thu, Oct 15, 2020 at 05:28:12PM +1100, matthew green wrote: you could try reverting most of our changes to this file and making sure you run with /proc mounted -o linux. ryo@ recently added additional /proc/cpuinfo support that should make this just work with the upstream version, but i haven't had chance to update and see if this is the case. > > I've confirmed that -mtune=native works fine at least for A53, > even if all the local changes to driver-aarch64.c is reverted. > I will commit it soon. > >>> If we go this route, we should make the relevant procfs nodes >>> independent >>> of -o linux. >> >> that would be fine by me. > > Nowadays, -o linux is turned on by default (unless nolinux is > specified explicitly). Still, native apps probably should not > depend on it. > > This needs MI changes to procfs, not MD to aarch64. Should we > enable /proc/cpuinfo unconditionally? I'm against the policy of restoring the /proc dependency for this corner case in one application. We need to upstream the NetBSD specific patches to mainline GCC. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/external/gpl3/gdb/dist/gdb
On 13.10.2020 11:14, Leonardo Taccari wrote: > Hello Kamil, > > Kamil Rytarowski writes: >> Module Name: src >> Committed By:kamil >> Date:Tue Oct 6 23:14:47 UTC 2020 >> >> Modified Files: >> src/external/gpl3/gdb/dist/gdb: inf-ptrace.c nbsd-nat.c >> >> Log Message: >> Undo local patches >> >> They are no longer needed (and are wrong). >> [...] > > This seems to break gdb, e.g. by starting debugging sleep(1): > > | % gdb sleep > | Reading symbols from sleep... > | Reading symbols from /usr/libdata/debug//bin/sleep.debug... > | (gdb) r > | Starting program: /bin/sleep > | /usr/src/external/gpl3/gdb/lib/libgdb/../../dist/gdb/thread.c:1309: > internal-error: void switch_to_thread(thread_info*): Assertion `thr != NULL' > failed. > | A problem internal to GDB has been detected, > | further debugging may prove unreliable. > | Quit this debugging session? (y or n) y > | > | This is a bug, please report it. For instructions, see: > | <https://www.gnu.org/software/gdb/bugs/>. > | > | /usr/src/external/gpl3/gdb/lib/libgdb/../../dist/gdb/thread.c:1309: > internal-error: void switch_to_thread(thread_info*): Assertion `thr != NULL' > failed. > | A problem internal to GDB has been detected, > | further debugging may prove unreliable. > | Create a core file of GDB? (y or n) y > | [ 240.7909538] sorry, pid 2675 was killed: orphaned traced process > | Abort (core dumped) > | Exit 134 > | % gdb -core gdb.core gdb > | Reading symbols from gdb... > | Reading symbols from /usr/libdata/debug//usr/bin/gdb.debug... > | [New process 1687] > | [New process 2908] > | [New process 2801] > | [New process 1688] > | [New process 1668] > | Core was generated by `gdb'. > | Program terminated with signal SIGABRT, Aborted. > | #0 0x76efa39833ba in _lwp_kill () from /usr/lib/libc.so.12 > | [Current thread is 1 (process 1687)] > | (gdb) bt > | #0 0x76efa39833ba in _lwp_kill () from /usr/lib/libc.so.12 > | #1 0x76efa3983879 in abort () at /usr/src/lib/libc/stdlib/abort.c:74 > | #2 0xd83814e9 in dump_core () at > /usr/src/external/gpl3/gdb/lib/libgdb/../../dist/gdb/utils.c:204 > | #3 0xd8386315 in internal_vproblem(internal_problem *, const char > *, int, const char *, typedef __va_list_tag __va_list_tag *) ( > | problem=problem@entry=0xd8bf05e0 , > file=, line=, fmt=, > ap=ap@entry=0x7f7fff084bc8) > | at /usr/src/external/gpl3/gdb/lib/libgdb/../../dist/gdb/utils.c:414 > | #4 0xd83864fb in internal_verror (file=, > line=, fmt=, ap=ap@entry=0x7f7fff084bc8) > | at /usr/src/external/gpl3/gdb/lib/libgdb/../../dist/gdb/utils.c:439 > | #5 0xd862e40a in internal_error (file=file@entry=0xd8758578 > "/usr/src/external/gpl3/gdb/lib/libgdb/../../dist/gdb/thread.c", > line=line@entry=1309, fmt=) > | at > /usr/src/external/gpl3/gdb/lib/libgdbsupport/../../dist/gdbsupport/errors.cc:55 > | #6 0xd83a96f5 in switch_to_thread (thr=0x0) at > /usr/src/external/gpl3/gdb/lib/libgdb/../../dist/gdb/thread.c:1309 > | #7 switch_to_thread (thr=) at > /usr/src/external/gpl3/gdb/lib/libgdb/../../dist/gdb/thread.c:1307 > | #8 0xd854e02e in startup_inferior > (proc_target=proc_target@entry=0xd8bf1c50 , > pid=pid@entry=2675, ntraps=ntraps@entry=1, > | last_waitstatus=last_waitstatus@entry=0x0, > last_ptid=last_ptid@entry=0x0) at > /usr/src/external/gpl3/gdb/lib/libgdb/../../dist/gdb/nat/fork-inferior.c:539 > | #9 0xd854ef07 in gdb_startup_inferior (pid=pid@entry=2675, > num_traps=num_traps@entry=1) at > /usr/src/external/gpl3/gdb/lib/libgdb/../../dist/gdb/fork-child.c:129 > | #10 0xd84f8592 in inf_ptrace_target::create_inferior > (this=this@entry=0xd8bf1c50 , > exec_file=exec_file@entry=0x76efa67946f0 "/bin/sleep", allargs=..., > | env=env@entry=0x76efa6b1e400, from_tty=from_tty@entry=1) at > /usr/src/external/gpl3/gdb/lib/libgdb/../../dist/gdb/inf-ptrace.c:117 > | #11 0xd82b98e0 in run_command_1 (args=, from_tty=1, > run_how=RUN_NORMAL) at > /usr/src/external/gpl3/gdb/lib/libgdb/../../dist/gdb/infcmd.c:493 > | #12 0xd83663d2 in cmd_func (cmd=, args= out>, from_tty=) at > /usr/src/external/gpl3/gdb/lib/libgdb/../../dist/gdb/cli/cli-decode.c:2181 > | #13 0xd83a402d in execute_command (p=, > p@entry=0x76efa6b5c020 "", from_tty=1) at > /usr/src/external/gpl3/gdb/lib/libgdb/../../dist/gdb/top.c:668 > | #14 0xd82d726c in command_handler (command=0x76efa6b5c020 "") at > /usr/src/external/gpl3/gdb/lib/libgdb/../../dist/gdb/event
Re: CVS commit: src/sys/arch/aarch64/aarch64
On 13.10.2020 09:04, Rin Okuyama wrote: > Module Name: src > Committed By: rin > Date: Tue Oct 13 07:04:49 UTC 2020 > > Modified Files: > src/sys/arch/aarch64/aarch64: exec_machdep.c > > Log Message: > BE32 binaries are no longer supported for ARMv7 and later, and > therefore for aarch64eb. > > Reject them with ENOEXEC, rather than causing illegal instruction > exceptions due to unexpected binary format. > > Not supported in general or on NetBSD? Big Endian 32bit is supported on Cavium ThunderX (at least on a selection of models if not all of them). signature.asc Description: OpenPGP digital signature
GCC TSan (Re: CVS commit: src/tests/usr.bin)
On 15.09.2020 07:03, Martin Husemann wrote: > On Mon, Sep 14, 2020 at 03:17:53PM +0000, Kamil Rytarowski wrote: >> Enable TSan tests for GCC and >32bit address space environments > > Since tsan does not work on all architectures, this is not a good idea. > It would be better to code it with an explicit list of architectures > supported. > > Martin > I've tried to mark the TSan parts that need porting as explicit failure, soo we can reduce the risk of shipping unported runtime. There are generally three such parts: - address space memory mappings - setjmp mapping of stack pointer - setjmp/longjmp assembly code I noted that not all ATF TSan tests pass for GCC amd64. I suspect that the runtime is still too old (even older than Clang-7 2 years ago, when we added the ATF tests). Another difference is that LLVM by default links the runtime statically and GCC dynamically. There is a crash with dl_iterate_phdr(3). This is possibly related to the crash with -pg. GCC requires to deliver .spec files for static linking of sanitizers. We need to generate libsanitizer.spec and install it to /usr/lib/. I will have a look into it. GCC9 also wants to install libasan_preinit.o, liblsan_preinit.o, libtsan_preinit.o. I'm going to prepare appropriate build rules for them. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/external/gpl3/gcc
On 12.09.2020 23:36, Joerg Sonnenberger wrote: > On Sat, Sep 12, 2020 at 10:24:16PM +0200, Kamil Rytarowski wrote: >> On 12.09.2020 22:06, Joerg Sonnenberger wrote: >>> On Fri, Sep 11, 2020 at 11:45:42PM +0200, Kamil Rytarowski wrote: >>>> On 11.09.2020 23:38, Joerg Sonnenberger wrote: >>>>> On Fri, Sep 11, 2020 at 04:07:24PM +0200, Kamil Rytarowski wrote: >>>>>> The current code is confusing, as it attempts to use unimplemented >>>>>> _PTHREAD_GETTCB_EXT() and in one place uses _lwp_getprivate_fast() in >>>>>> other _lwp_getprivate(). This caused my confusion... as I assumed that >>>>>> _lwp_getprivate_fast() is internal and _lwp_getprivate() for public >>>>>> consumption. >>>>> >>>>> _PTHREAD_GETTCB_EXT is a rump hack. There is no _lwp_getprivate_fast. >>>>> There is __lwp_getprivate_fast, which originally wasn't implemented on >>>>> architectures without a fast path (like VAX). Nowadays, all functional >>>>> ports provide either __lwp_getprivate_fast (potentially with a fall-back >>>>> to the system call) or __lwp_gettcb_fast. The difference is whether the >>>>> TLS register is biased or not. >>>>> >>>> >>>> Do you agree with this patch: >>>> >>>> http://netbsd.org/~kamil/patch-00278-_rtld_tls_self.txt >>> >>> No, I don't see the point. >>> >> >> What's the alternative to use in 3rd party code? > > Why do you need an alternative? > I need tls_tcb of the calling thread. https://github.com/llvm/llvm-project/blob/15b37e1cfa5f09af376a47a1bc67d67bb5c7848b/compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp#L406 https://github.com/llvm/llvm-project/blob/15b37e1cfa5f09af376a47a1bc67d67bb5c7848b/compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp#L458 What is the interface (ideally MI) for this for 3rd party code? > Joerg > signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/external/gpl3/gcc
On 12.09.2020 22:06, Joerg Sonnenberger wrote: > On Fri, Sep 11, 2020 at 11:45:42PM +0200, Kamil Rytarowski wrote: >> On 11.09.2020 23:38, Joerg Sonnenberger wrote: >>> On Fri, Sep 11, 2020 at 04:07:24PM +0200, Kamil Rytarowski wrote: >>>> The current code is confusing, as it attempts to use unimplemented >>>> _PTHREAD_GETTCB_EXT() and in one place uses _lwp_getprivate_fast() in >>>> other _lwp_getprivate(). This caused my confusion... as I assumed that >>>> _lwp_getprivate_fast() is internal and _lwp_getprivate() for public >>>> consumption. >>> >>> _PTHREAD_GETTCB_EXT is a rump hack. There is no _lwp_getprivate_fast. >>> There is __lwp_getprivate_fast, which originally wasn't implemented on >>> architectures without a fast path (like VAX). Nowadays, all functional >>> ports provide either __lwp_getprivate_fast (potentially with a fall-back >>> to the system call) or __lwp_gettcb_fast. The difference is whether the >>> TLS register is biased or not. >>> >> >> Do you agree with this patch: >> >> http://netbsd.org/~kamil/patch-00278-_rtld_tls_self.txt > > No, I don't see the point. > What's the alternative to use in 3rd party code? > Joerg > signature.asc Description: OpenPGP digital signature
Re: CVS commit: src
On 11.09.2020 06:57, Rin Okuyama wrote: > On 2020/09/10 18:28, Kamil Rytarowski wrote: >> On 10.09.2020 10:50, Rin Okuyama wrote: >>> On 2020/09/10 16:41, Kamil Rytarowski wrote: >>>> On 10.09.2020 03:53, Rin Okuyama wrote: >>>>> Module Name: src >>>>> Committed By: rin >>>>> Date: Thu Sep 10 01:53:22 UTC 2020 >>>>> >>>>> Modified Files: >>>>> src/distrib/sets/lists/base: mi >>>>> src/distrib/sets/lists/comp: mi >>>>> src/sys/dev: Makefile >>>>> >>>>> Log Message: >>>>> Unconditionally install kernel headers for iSCSI as required by >>>>> sanitizer shipped with GCC9. >>>>> >>>>> Fix build release with HAVE_GCC=9 for sun2, where MKISCSI=no by >>>>> default. >>>>> >>>> >>>> Please redo this commit with __has_include(), example: >>>> >>>> https://github.com/llvm/llvm-project/commit/7f6b25ad1bb3f8057655a9bad2a3b69621f4a543#diff-fa188a123646bb8c30d7fa22d61ef680 >>>> >>>> >>>> >>> >>> Hmm, I'm not sure whether it is worth the maintenance cost and >>> complexities it adds... Is there any reason why we should not >>> install these headers? >>> >> >> The headers are optional and sanitizers shall not dictate what headers >> are installed. >> >> Meanwhile, I'm going to do this in upstream LLVM. > > Yes, I really appreciate your continuous efforts in syncing our > codes with upstream. > > However, still, there is ``the maintenance cost'' in our local > repository. With __has_include(), we must maintain two copies of > headers in the tree. This is dangerous potentially. > There is no danger in terms of security. > My question was whether it is worth the risk not to install few > number of small headers. > I've updated the LLVM interceptors. Please revert. > Thanks, > rin signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys
On 11.09.2020 17:16, Roy Marples wrote: > Module Name: src > Committed By: roy > Date: Fri Sep 11 15:16:00 UTC 2020 > > Modified Files: > src/sys/net: if_llatbl.c > src/sys/netinet: if_arp.c if_inarp.h tcp_input.c > > Log Message: > ARP: Use ND rather than our own. > > This brings the benefit of Neighbour Unreachability Detection which is > something ARP sorely lacks. > > The new timings mirror those of IPv6 and are adjustable via sysctl(8). > Unlike IPv6 ND, these are global and not per interface. > > This change broke rump: /public/netbsd-9/tooldir.NetBSD-9.99.72-amd64/lib/gcc/x86_64--netbsd/9.3.0/../../../../x86_64--netbsd/bin/ld: /public/netbsd-9/destdir.amd64/usr/lib/librumpnet_net.so: undefined reference to `rumpns_nd_set_timer' /public/netbsd-9/tooldir.NetBSD-9.99.72-amd64/lib/gcc/x86_64--netbsd/9.3.0/../../../../x86_64--netbsd/bin/ld: /public/netbsd-9/destdir.amd64/usr/lib/librumpnet_net.so: undefined reference to `rumpns_nd_resolve' /public/netbsd-9/tooldir.NetBSD-9.99.72-amd64/lib/gcc/x86_64--netbsd/9.3.0/../../../../x86_64--netbsd/bin/ld: /public/netbsd-9/destdir.amd64/usr/lib/librumpnet_net.so: undefined reference to `rumpns_nd_nud_hint' /public/netbsd-9/tooldir.NetBSD-9.99.72-amd64/lib/gcc/x86_64--netbsd/9.3.0/../../../../x86_64--netbsd/bin/ld: /public/netbsd-9/destdir.amd64/usr/lib/librumpnet_net.so: undefined reference to `rumpns_nd_attach_domain' collect2: error: ld returned 1 exit status signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/external/gpl3/gcc
On 11.09.2020 23:38, Joerg Sonnenberger wrote: > On Fri, Sep 11, 2020 at 04:07:24PM +0200, Kamil Rytarowski wrote: >> The current code is confusing, as it attempts to use unimplemented >> _PTHREAD_GETTCB_EXT() and in one place uses _lwp_getprivate_fast() in >> other _lwp_getprivate(). This caused my confusion... as I assumed that >> _lwp_getprivate_fast() is internal and _lwp_getprivate() for public >> consumption. > > _PTHREAD_GETTCB_EXT is a rump hack. There is no _lwp_getprivate_fast. > There is __lwp_getprivate_fast, which originally wasn't implemented on > architectures without a fast path (like VAX). Nowadays, all functional > ports provide either __lwp_getprivate_fast (potentially with a fall-back > to the system call) or __lwp_gettcb_fast. The difference is whether the > TLS register is biased or not. > Do you agree with this patch: http://netbsd.org/~kamil/patch-00278-_rtld_tls_self.txt And then, using _rtld_tls_self() in sanitizers (and wherever someone finds it useful)? As an alternative we will use __lwp_gettcb_fast() or __lwp_getprivate_fast() manually in 3rd party code, which seems fragile. > Joerg > signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/external/gpl3/gcc
On 11.09.2020 07:13, Rin Okuyama wrote:
> Hi again,
>
> On 2020/09/10 21:53, Kamil Rytarowski wrote:
>> Module Name: src
>> Committed By: kamil
>> Date: Thu Sep 10 12:53:06 UTC 2020
>>
>> Modified Files:
>> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common:
>> sanitizer_linux_libcdep.cc
>> src/external/gpl3/gcc/lib: Makefile.sanitizer
>>
>> Log Message:
>> Avoid using internal RTLD/libpthread/libc symbol in sanitizers
>>
> ...
>> Index:
>> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
>>
>> diff -u
>> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc:1.15
>> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc:1.16
>>
>> ---
>> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc:1.15
>>
>> Mon Sep 7 07:10:43 2020
>> +++
>> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
>>
>> Thu Sep 10 12:53:05 2020
>> @@ -47,6 +47,7 @@
>> #if SANITIZER_NETBSD
>> #include
>> #include
>> +#include
>> #endif
>> #if SANITIZER_SOLARIS
>> @@ -417,13 +418,7 @@ uptr ThreadSelf() {
>> #if SANITIZER_NETBSD
>> static struct tls_tcb * ThreadSelfTlsTcb() {
>> - struct tls_tcb * tcb = NULL;
>> -# ifdef __HAVE___LWP_GETTCB_FAST
>> - tcb = (struct tls_tcb *)__lwp_gettcb_fast();
>> -# elif defined(__HAVE___LWP_GETPRIVATE_FAST)
>> - tcb = (struct tls_tcb *)__lwp_getprivate_fast();
>> -# endif
>> - return tcb;
>> + return (struct tls_tcb *)_lwp_getprivate();
>> }
>> uptr ThreadSelf() {
>>
>
> This change breaks at least mips and powerpc, in which the return value of
> __lwp_getprivate(2), i.e., curlwp->l_private is not tcb address itself, but
> biased one. On the other hand, the return value of __lwp_gettcb_fast() is
> unbiased address; see sys/arch/{mips,powerpc}/include/mcontext.h.
>
> For powerpc, I recently attempted to change l_private to store tcb address
> itself:
>
> http://www.nerv.org/netbsd/?q=id:20200621T004000Z.95c1a18070b53713ce2c763df7f40743bf74172c
>
>
> But I reverted it soon as requested by joerg:
>
> http://www.nerv.org/netbsd/?q=id:20200622T053457Z.05db3be87b5ad499f5d1adba755bc573fd241c87
>
>
> His reasoning was that kernel must not know the ABI details in userland.
> I fully agree with this. See above links for more details.
>
> Thanks,
> rin
Thank you for noting it!
This is strange as I assumed that _lwp_getprivate() returns always the
correct private pointer and it is abstraction over fast ABI specific
calls . Also the usage of _lwp_getprivate() was suggested by Joerg back
then in sanitizers.
So we want exported to userland functionality to get the tls_tcb
pointer, something without using the internal RTLS/LIBPTHREAD/LIBC
namespaces.
The current code is confusing, as it attempts to use unimplemented
_PTHREAD_GETTCB_EXT() and in one place uses _lwp_getprivate_fast() in
other _lwp_getprivate(). This caused my confusion... as I assumed that
_lwp_getprivate_fast() is internal and _lwp_getprivate() for public
consumption.
https://nxr.netbsd.org/xref/src/lib/libpthread/pthread_int.h#266
263 static inline pthread_t __constfunc
264 pthread__self(void)
265 {
266 #if defined(_PTHREAD_GETTCB_EXT)
267 struct tls_tcb * const tcb = _PTHREAD_GETTCB_EXT();
268 #elif defined(__HAVE___LWP_GETTCB_FAST)
269 struct tls_tcb * const tcb = __lwp_gettcb_fast();
270 #else
271 struct tls_tcb * const tcb = __lwp_getprivate_fast();
272 #endif
273 return (pthread_t)tcb->tcb_pthread;
274 }
https://nxr.netbsd.org/xref/src/lib/libpthread/pthread.c#1268
1268 #if defined(_PTHREAD_GETTCB_EXT)
1269 pthread__main->pt_tls = _PTHREAD_GETTCB_EXT();
1270 #elif defined(__HAVE___LWP_GETTCB_FAST)
1271 pthread__main->pt_tls = __lwp_gettcb_fast();
1272 #else
1273 pthread__main->pt_tls = _lwp_getprivate();
1274 #endif
1275 pthread__main->pt_tls->tcb_pthread = pthread__main;
https://nxr.netbsd.org/xref/src/libexec/ld.elf_so/tls.c#294
293 #ifdef __HAVE___LWP_GETTCB_FAST
294 struct tls_tcb * const tcb = __lwp_gettcb_fast();
295 #else
296 struct tls_tcb * const tcb = __lwp_getprivate_fast();
297 #endif
1. Could we please synchronize above three code chunks, avoiding the
situation of having each of them implemented differently?
2. Could we please export _rtld_tls_self() or something similar and
register in ?
Does this patch look good?
https://w
Re: CVS commit: src
On 10.09.2020 10:50, Rin Okuyama wrote: > On 2020/09/10 16:41, Kamil Rytarowski wrote: >> On 10.09.2020 03:53, Rin Okuyama wrote: >>> Module Name: src >>> Committed By: rin >>> Date: Thu Sep 10 01:53:22 UTC 2020 >>> >>> Modified Files: >>> src/distrib/sets/lists/base: mi >>> src/distrib/sets/lists/comp: mi >>> src/sys/dev: Makefile >>> >>> Log Message: >>> Unconditionally install kernel headers for iSCSI as required by >>> sanitizer shipped with GCC9. >>> >>> Fix build release with HAVE_GCC=9 for sun2, where MKISCSI=no by default. >>> >> >> Please redo this commit with __has_include(), example: >> >> https://github.com/llvm/llvm-project/commit/7f6b25ad1bb3f8057655a9bad2a3b69621f4a543#diff-fa188a123646bb8c30d7fa22d61ef680 >> >> > > Hmm, I'm not sure whether it is worth the maintenance cost and > complexities it adds... Is there any reason why we should not > install these headers? > The headers are optional and sanitizers shall not dictate what headers are installed. Meanwhile, I'm going to do this in upstream LLVM. > Thanks, > rin signature.asc Description: OpenPGP digital signature
Re: CVS commit: src
On 10.09.2020 03:53, Rin Okuyama wrote: > Module Name: src > Committed By: rin > Date: Thu Sep 10 01:53:22 UTC 2020 > > Modified Files: > src/distrib/sets/lists/base: mi > src/distrib/sets/lists/comp: mi > src/sys/dev: Makefile > > Log Message: > Unconditionally install kernel headers for iSCSI as required by > sanitizer shipped with GCC9. > > Fix build release with HAVE_GCC=9 for sun2, where MKISCSI=no by default. > Please redo this commit with __has_include(), example: https://github.com/llvm/llvm-project/commit/7f6b25ad1bb3f8057655a9bad2a3b69621f4a543#diff-fa188a123646bb8c30d7fa22d61ef680 signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common
On 05.09.2020 15:35, matthew green wrote:
> Module Name: src
> Committed By: mrg
> Date: Sat Sep 5 13:35:55 UTC 2020
>
> Modified Files:
> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common:
> sanitizer_linux.cc sanitizer_linux.h sanitizer_linux_libcdep.cc
> sanitizer_platform_limits_netbsd.cc sanitizer_syscall_generic.inc
>
> Log Message:
> fix various merge botches; we may need to re-port the ThreadLister code.
>
LSan was ported and upstreamed post GCC-9:
https://github.com/llvm/llvm-project/commit/1b58389428ed07a7322ba9c2bcaeec99807f9457
https://github.com/llvm/llvm-project/commit/5fe1e55d35413b1904cfcf16ec15495398921fe5
https://github.com/llvm/llvm-project/commit/8827047551570b7ed7088765c3de2a8cce6823b8
https://github.com/llvm/llvm-project/commit/983d7ddd0b278b45d815cbac9197205b39c4860a
https://github.com/llvm/llvm-project/commit/fc356dcc11c10003ff22acff667b0a9f5e6c1e0f
https://github.com/llvm/llvm-project/commit/3a189bac9bb111c9a59339015ab0d4e2fed735f4
https://github.com/llvm/llvm-project/commit/02519fc7a6f8c528f67975a9f78ce64dabf402b4
And possibly something else:
https://github.com/llvm/llvm-project/commits?author=krytarowski
>
> To generate a diff of this commit:
> cvs rdiff -u -r1.33 -r1.34 \
>
> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_linux.cc
> cvs rdiff -u -r1.9 -r1.10 \
> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_linux.h
> cvs rdiff -u -r1.12 -r1.13 \
>
> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
> cvs rdiff -u -r1.1.1.2 -r1.2 \
>
> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_platform_limits_netbsd.cc
> cvs rdiff -u -r1.8 -r1.9 \
>
> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_syscall_generic.inc
>
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
>
>
> Modified files:
>
> Index:
> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_linux.cc
> diff -u
> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_linux.cc:1.33
>
> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_linux.cc:1.34
> ---
> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_linux.cc:1.33
> Sat Sep 5 09:12:32 2020
> +++
> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_linux.cc
> Sat Sep 5 13:35:55 2020
> @@ -41,8 +41,6 @@
> #undef stat
> #endif
>
> -#endif // SANITIZER_LINUX
> -
> #if SANITIZER_NETBSD
> #include
> #endif
> @@ -980,7 +978,6 @@ ThreadLister::ThreadLister(pid_t pid) :
>if (internal_iserror(descriptor_)) {
> Report("Can't open /proc/%d/task for reading.\n", pid);
>}
> -#endif
> }
>
> ThreadLister::Result ThreadLister::ListThreads(
> @@ -1055,10 +1052,8 @@ bool ThreadLister::IsAlive(int tid) {
> }
>
> ThreadLister::~ThreadLister() {
> -#ifndef SANITIZER_NETBSD
>if (!internal_iserror(descriptor_))
> internal_close(descriptor_);
> -#endif
> }
> #endif
>
>
> Index:
> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_linux.h
> diff -u
> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_linux.h:1.9
>
> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_linux.h:1.10
> ---
> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_linux.h:1.9
> Sat Sep 5 09:12:32 2020
> +++
> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_linux.h
> Sat Sep 5 13:35:55 2020
> @@ -69,6 +69,8 @@ uptr internal_clone(int (*fn)(void *), v
> #endif
> #elif SANITIZER_FREEBSD
> void internal_sigdelset(__sanitizer_sigset_t *set, int signum);
> +#elif SANITIZER_NETBSD
> +uptr internal_prctl(int option, uptr arg2, uptr arg3, uptr arg4, uptr arg5);
> #endif // SANITIZER_LINUX
>
> #ifdef SANITIZER_NETBSD
>
> Index:
> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
> diff -u
> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc:1.12
>
> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc:1.13
> ---
> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc:1.12
> Sat Sep 5 09:12:32 2020
> +++
> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cc
>Sat Sep 5 13:35:55 2020
> @@ -72,6 +72,10 @@ struct __sanitizer::linux_dirent {
> #include
> #endif
>
> +#if SANITIZER_NETBSD
> +#include
> +#endif
> +
> namespace __sanitizer {
>
> SANITIZER_WEAK_ATTRIBUTE int
>
> Index:
> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_platform_limits_netbsd.cc
> diff -u
> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sanitizer_platform_limits_netbsd.cc:1.1.1.2
>
> src/external/gpl3/gcc/dist/libsanitizer/sanitizer_common/sani
Re: CVS commit: src/sys/kern
On 02.08.2020 17:50, Taylor R Campbell wrote:
>> Date: Sun, 2 Aug 2020 17:35:06 +0200
>> From: Kamil Rytarowski
>>
>> On 02.08.2020 16:44, Taylor R Campbell wrote:
>>>> Date: Sun, 2 Aug 2020 16:04:15 +0200
>>>> From: Kamil Rytarowski
>>>>
>>>> On 02.08.2020 15:57, Taylor R Campbell wrote:
>>>>> But it sounds like the original motivation is that it triggered
>>>>> -Wvla...which frankly strikes me as a compiler bug since there's
>>>>> obviously no actual VLA created in sizeof; as far as I can tell
>>>>> there's no semantic difference between sizeof(device_t[n]) and
>>>>> sizeof(device_t) * n.
>>>>
>>>> This is not true:
>>>
>>> Which part of what I said are you claiming is not true, and what are
>>> you illustrating with the example program below?
>>
>> Calling it a compiler bug.
>>
>> Clang behaves the same way.
>>
>> $ clang -Wvla test.c
>> test.c:6:37: warning: variable length array used [-Wvla]
>> printf("sizeof = %zu\n", sizeof(int[argc]));
>>^
>> 1 warning generated.
>>
>> Creating VLA is not needed for using it as an intermediate. In practice
>> in most/all cases it is optimized and actual VLA is not allocated.
>
> This is not a matter of optimization in practice. It's absolutely not
> an `intermediate' at all -- there is no VLA created, period, any more
> than sizeof(malloc(1)) causes any actual call to malloc or sizeof(*p)
> causes any dereference of a pointer.
>
> This makes -Wvla less useful as a tool, because apparently it flags
> code that unquestionably does not have any bad effects of VLAs. This
> happens because -Wvla is dumb -- it just looks for the syntax, not for
> the semantics of creating VLAs. That's why I call it a bug -- it
> raises a false positive that makes it less useful.
>
Compilers warns about VLA using ("variable length array used"), not
creating. There is no distinct compiler warning to discriminate VLA
creating from usage.
Anyway, code just using VLA is not that frequent, avoiding it is rather
simple and VLA can be avoided altogether (at least for non-external).
signature.asc
Description: OpenPGP digital signature
Re: CVS commit: src/sys/kern
On 02.08.2020 16:44, Taylor R Campbell wrote:
>> Date: Sun, 2 Aug 2020 16:04:15 +0200
>> From: Kamil Rytarowski
>>
>> On 02.08.2020 15:57, Taylor R Campbell wrote:
>>> But it sounds like the original motivation is that it triggered
>>> -Wvla...which frankly strikes me as a compiler bug since there's
>>> obviously no actual VLA created in sizeof; as far as I can tell
>>> there's no semantic difference between sizeof(device_t[n]) and
>>> sizeof(device_t) * n.
>>
>> This is not true:
>
> Which part of what I said are you claiming is not true, and what are
> you illustrating with the example program below?
>
Calling it a compiler bug.
Clang behaves the same way.
$ clang -Wvla test.c
test.c:6:37: warning: variable length array used [-Wvla]
printf("sizeof = %zu\n", sizeof(int[argc]));
^
1 warning generated.
Creating VLA is not needed for using it as an intermediate. In practice
in most/all cases it is optimized and actual VLA is not allocated.
> It seems to illustrate that sizeof(int[argc]) does exactly what one
> would expect it to do -- return the size of an argc-length array of
> ints, just like sizeof(int) * argc does.
>
>
The result is the same and I find the change as beneficial.
>
>> #include
>>
>> int
>> main(int argc, char **argv)
>> {
>> printf("sizeof = %zu\n", sizeof(int[argc]));
>> return 0;
>> }
>>
>> $ ./a.out
>>
>> sizeof = 4
>> $ ./a.out 12 3
>> sizeof = 12
>> $ ./a.out 12 3 45 6
>> sizeof = 20
signature.asc
Description: OpenPGP digital signature
Re: CVS commit: src/sys/kern
On 02.08.2020 16:25, Paul Goyette wrote:
> On Sun, 2 Aug 2020, Kamil Rytarowski wrote:
>
>> On 02.08.2020 15:57, Taylor R Campbell wrote:
>>> But it sounds like the original motivation is that it triggered
>>> -Wvla...which frankly strikes me as a compiler bug since there's
>>> obviously no actual VLA created in sizeof; as far as I can tell
>>> there's no semantic difference between sizeof(device_t[n]) and
>>> sizeof(device_t) * n.
>>>
>>
>> This is not true:
>>
>> #include
>>
>> int
>> main(int argc, char **argv)
>> {
>> printf("sizeof = %zu\n", sizeof(int[argc]));
>> return 0;
>> }
>>
>> $ ./a.out
>>
>> sizeof = 4
>> $ ./a.out 12 3
>> sizeof = 12
>> $ ./a.out 12 3 45 6
>> sizeof = 20
>
> Modifying your example slightly, I print both variations:
>
> #include
>
> int
> main(int argc, char **argv)
> {
> printf("sizeof = %zu\t%zu\n", sizeof(int[argc]), sizeof(int) * argc);
> return 0;
> }
> speedy:paul {653} ./a.out
> sizeof = 4 4
> speedy:paul {654} ./a.out 12 3
> sizeof = 12 12
> speedy:paul {655} ./a.out 12 3 45 6
> sizeof = 20 20
>
>
> Looks the same to me!
>
The result is the same, but one uses VLA (at least formally), other not.
>
> ++--+---+
> | Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
> | (Retired) | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com |
> | Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org |
> ++--+---+
signature.asc
Description: OpenPGP digital signature
Re: CVS commit: src/sys/kern
On 02.08.2020 15:57, Taylor R Campbell wrote:
> But it sounds like the original motivation is that it triggered
> -Wvla...which frankly strikes me as a compiler bug since there's
> obviously no actual VLA created in sizeof; as far as I can tell
> there's no semantic difference between sizeof(device_t[n]) and
> sizeof(device_t) * n.
>
This is not true:
#include
int
main(int argc, char **argv)
{
printf("sizeof = %zu\n", sizeof(int[argc]));
return 0;
}
$ ./a.out
sizeof = 4
$ ./a.out 12 3
sizeof = 12
$ ./a.out 12 3 45 6
sizeof = 20
signature.asc
Description: OpenPGP digital signature
Re: CVS commit: src/tests/lib/libc/sys
On 22.06.2020 04:51, Rin Okuyama wrote: > Module Name: src > Committed By: rin > Date: Mon Jun 22 02:51:07 UTC 2020 > > Modified Files: > src/tests/lib/libc/sys: t_ptrace_signal_wait.h t_ptrace_wait.h > > Log Message: > Turn trigger_fpe() back to integer division by zero for a while > until QEMU bug #1668041 is fixed: > > https://bugs.launchpad.net/qemu/+bug/1668041 > > by which floating-point division by zero is not trapped correctly > both on amd64 and i386. > > Skip *_crash_fpe tests on powerpc, where integer division by zero > is never trapped. > The intention of this test is just detecting SIGFPE. If the si_code value is various on different architectures (like one traps on integers other one on floats), we shall just drop the asserts for si_code and adapt the trigger_fpe() function to cover promptly more ports. The qemu bug should be fixed nonetheless. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/rump/include/rump
On 16.06.2020 14:23, Kamil Rytarowski wrote: > On 16.06.2020 12:25, J. Hannken-Illjes wrote: >>> On 15. Jun 2020, at 01:38, Kamil Rytarowski wrote: >>> >>> Module Name:src >>> Committed By: kamil >>> Date: Sun Jun 14 23:38:25 UTC 2020 >>> >>> Modified Files: >>> src/sys/rump/include/rump: rump.h >>> >>> Log Message: >>> Remove old compat include of rump_syscallshotgun.h >>> >>> It was separated in 2016 and is no longer needed. >>> >>> >>> To generate a diff of this commit: >>> cvs rdiff -u -r1.71 -r1.72 src/sys/rump/include/rump/rump.h >>> >>> Please note that diffs are not public domain; they are subject to the >>> copyright notices on the relevant files. >> >> This broke most or all NFS tests on ATF (see attached list). >> >> Please revert or investigate. >> >> -- >> J. Hannken-Illjes - hann...@eis.cs.tu-bs.de - TU Braunschweig (Germany) >> > > I'm looking into it. > Should be fixed. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/rump/include/rump
On 16.06.2020 12:25, J. Hannken-Illjes wrote: >> On 15. Jun 2020, at 01:38, Kamil Rytarowski wrote: >> >> Module Name: src >> Committed By:kamil >> Date:Sun Jun 14 23:38:25 UTC 2020 >> >> Modified Files: >> src/sys/rump/include/rump: rump.h >> >> Log Message: >> Remove old compat include of rump_syscallshotgun.h >> >> It was separated in 2016 and is no longer needed. >> >> >> To generate a diff of this commit: >> cvs rdiff -u -r1.71 -r1.72 src/sys/rump/include/rump/rump.h >> >> Please note that diffs are not public domain; they are subject to the >> copyright notices on the relevant files. > > This broke most or all NFS tests on ATF (see attached list). > > Please revert or investigate. > > -- > J. Hannken-Illjes - hann...@eis.cs.tu-bs.de - TU Braunschweig (Germany) > I'm looking into it. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/common/lib/libprop
On 12.06.2020 03:09, Joerg Sonnenberger wrote: > On Fri, Jun 12, 2020 at 02:59:40AM +0200, Kamil Rytarowski wrote: >> On 12.06.2020 02:07, Joerg Sonnenberger wrote: >>> On Fri, Jun 12, 2020 at 01:28:15AM +0200, Kamil Rytarowski wrote: >>>> Please list legitimate false positives. There is practically nothing >>>> like that possible for using deprecated APIs (at least kept longer >>>> term). Besides that, the report shall be lowered to warning (like it >>>> used to be for Clang). >>> >>> Build a random KDE package and see warnings about XHR symbols? >>> >>> Joerg >>> >> >> XDR? >> >> $ grep -ir xdr .|grep warn >> ./libc/yp/xdryp.c:__warn_references(xdr_domainname, >> ./libc/yp/xdryp.c:"warning: this program uses xdr_domainname(), >> which is deprecated and buggy.") >> ./libc/yp/xdryp.c:__warn_references(xdr_peername, >> ./libc/yp/xdryp.c:"warning: this program uses xdr_peername(), which >> is deprecated and buggy.") >> ./libc/yp/xdryp.c:__warn_references(xdr_mapname, >> ./libc/yp/xdryp.c:"warning: this program uses xdr_mapname(), which >> is deprecated and buggy.") >> >> KDE4? > > No, KDE5. > >> >> After grepping, I don't see relevant users of these APIs. There is >> something around Python that has a NIS/YP module. > > Exactly, nothing in Qt uses it, but it *still* triggers the warning. > My first suspect is python. Anyway it shall be a warning and problem solved. > Joerg > signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/common/lib/libprop
On 12.06.2020 02:07, Joerg Sonnenberger wrote: > On Fri, Jun 12, 2020 at 01:28:15AM +0200, Kamil Rytarowski wrote: >> Please list legitimate false positives. There is practically nothing >> like that possible for using deprecated APIs (at least kept longer >> term). Besides that, the report shall be lowered to warning (like it >> used to be for Clang). > > Build a random KDE package and see warnings about XHR symbols? > > Joerg > XDR? $ grep -ir xdr .|grep warn ./libc/yp/xdryp.c:__warn_references(xdr_domainname, ./libc/yp/xdryp.c:"warning: this program uses xdr_domainname(), which is deprecated and buggy.") ./libc/yp/xdryp.c:__warn_references(xdr_peername, ./libc/yp/xdryp.c:"warning: this program uses xdr_peername(), which is deprecated and buggy.") ./libc/yp/xdryp.c:__warn_references(xdr_mapname, ./libc/yp/xdryp.c:"warning: this program uses xdr_mapname(), which is deprecated and buggy.") KDE4? After grepping, I don't see relevant users of these APIs. There is something around Python that has a NIS/YP module. Even if this is the case, it shall be a warning (like it used to be). signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/common/lib/libprop
On 12.06.2020 01:11, Joerg Sonnenberger wrote: > On Fri, Jun 12, 2020 at 12:58:45AM +0200, Kamil Rytarowski wrote: >> On 12.06.2020 00:55, Christos Zoulas wrote: >>> In article <20200611222544.6d3a6f...@cvs.netbsd.org>, >>> Joerg Sonnenberger wrote: >>>> -=-=-=-=-=- >>>> >>>> Module Name: src >>>> Committed By: joerg >>>> Date: Thu Jun 11 22:25:44 UTC 2020 >>>> >>>> Modified Files: >>>>src/common/lib/libprop: prop_object_impl.h >>>> >>>> Log Message: >>>> Unbreak clang builds by removing questionable linker warning sections >>>> trggered all over the place. >>> >>> Why don't you do this just for clang, so that we can use gcc to track the >>> remaining ones down and finish the job? Now we can't easily find them. >>> >>> christos >>> >> >> Can we please revert this and fix clang? >> >> I'm strongly for linker warnings as they catch real issues. > > Repeating that statement doesn't make it true. The amount of legit > problems found by them is dwarfed by far by the number of false > positives seen. That's complete ignoring basic QoI issues like "where is > this actually triggered" and no "I know, shut up". > > Joerg > Please list legitimate false positives. There is practically nothing like that possible for using deprecated APIs (at least kept longer term). Besides that, the report shall be lowered to warning (like it used to be for Clang). signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/common/lib/libprop
On 12.06.2020 00:55, Christos Zoulas wrote: > In article <20200611222544.6d3a6f...@cvs.netbsd.org>, > Joerg Sonnenberger wrote: >> -=-=-=-=-=- >> >> Module Name: src >> Committed By:joerg >> Date:Thu Jun 11 22:25:44 UTC 2020 >> >> Modified Files: >> src/common/lib/libprop: prop_object_impl.h >> >> Log Message: >> Unbreak clang builds by removing questionable linker warning sections >> trggered all over the place. > > Why don't you do this just for clang, so that we can use gcc to track the > remaining ones down and finish the job? Now we can't easily find them. > > christos > Can we please revert this and fix clang? I'm strongly for linker warnings as they catch real issues. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src
On 07.06.2020 22:57, Joerg Sonnenberger wrote: > On Sun, Jun 07, 2020 at 01:27:48PM -0700, Jason Thorpe wrote: >> >>> On Jun 7, 2020, at 1:22 PM, Joerg Sonnenberger wrote: >>> >>> On Sat, Jun 06, 2020 at 09:26:00PM +, Jason R Thorpe wrote: ==> Deprecate mutable prop_data(3) and prop_string(3) objects. The old APIs that support them still exist, but will now produce link-time warnings when used. >>> >>> Please replace them with proper deprecration annotation in the headers. >> >> Example? > > https://gcc.gnu.org/onlinedocs/gcc-4.7.0/gcc/Function-Attributes.html#index-g_t_0040code_007bdeprecated_007d-attribute_002e-2502 > > Joerg > Warnings from headers are worse as whenever a symbol is not pulled from the headers the application has no idea that it is using an old deprecated or broken symbol. (I caught with the linker time warning many issues in .NET on NetBSD) So, I'm against this. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src
On 07.06.2020 22:27, Jason Thorpe wrote: > >> On Jun 7, 2020, at 1:22 PM, Joerg Sonnenberger wrote: >> >> On Sat, Jun 06, 2020 at 09:26:00PM +, Jason R Thorpe wrote: >>> ==> Deprecate mutable prop_data(3) and prop_string(3) objects. The old >>>APIs that support them still exist, but will now produce link-time >>>warnings when used. >> >> Please replace them with proper deprecration annotation in the headers. > > Example? > This is our practice and it is very useful in switching users. If that is obnoxious (in reality informative), then the purpose has been served accordingly. > -- thorpej > signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/arch/x86/x86
On 06.06.2020 09:42, Simon Burge wrote: > "Kamil Rytarowski" wrote: > >> Module Name: src >> Committed By:kamil >> Date:Fri Jun 5 21:48:04 UTC 2020 >> >> Modified Files: >> >> src/sys/arch/x86/x86: cpu_rng.c >> >> Log Message: >> >> Change const unsigned to preprocessor define >> >> Fixes GCC -O0 build with the stack protector. > > Surely a gcc bug? This almost certainly needs an > /* XXX gcc stack protector -O0 bug */ comment and > possibly an entry in doc/HACKS as well otherwise > someone will come along later and de-uglify this > change. > > Cheers, > Simon. > This is not really a GCC bug, as C const is not constexpr. It's also not the only place with such logic and such workaround. C++ fixed it and have real const. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src
On 05.06.2020 04:06, Robert Elz wrote: > Date:Fri, 5 Jun 2020 01:50:47 +0200 > From: Kamil Rytarowski > Message-ID: > > | What happened to RT signal names? > | > | I'm not sure what's wrong as this code works under a debugger. > > No idea right now, but I will take a look. > > Which shell are you using for that? And which version. That's > relevant as kill is almost always a built in, so when you just > run "kill -l" you're getting whatever the shell knows, but when you > run it under the debugger, you're getting /bin/kill (you could also > try running that explicitly, see if it makes a difference). > > kre > I can see the problem now. It's a fault in ksh(1). Thanks for pointing it out! signature.asc Description: OpenPGP digital signature
Re: CVS commit: src
On 09.05.2017 13:14, Robert Elz wrote: > Module Name: src > Committed By: kre > Date: Tue May 9 11:14:16 UTC 2017 > > Modified Files: > src/distrib/sets/lists/base: shl.mi > src/distrib/sets/lists/comp: mi > src/distrib/sets/lists/debug: shl.mi > src/include: signal.h > src/lib/libc: shlib_version > src/lib/libc/gen: Makefile.inc > Added Files: > src/lib/libc/gen: signalname.3 signalname.c signalnext.c signalnumber.c > > Log Message: > Add the new signalname/signalnext/signalnumber interface to libc. > > This as discussed on current-users in the thread > entitled: > Proposal: new libc/libutil functions to map SIG <-> "" > that can be found (starting at): > http://mail-index.netbsd.org/current-users/2017/04/28/msg031600.html > > These functions provide the mechanism to enable applications > to divorce themselves from internal details of the signal > implementation. > > Libc minor bumped, prototypes in , sets lists updated (and sorted). > > One and all: feel free to improve the sources & man page (etc), but > please do not change the function signatures without discussion. > I have got a strange behavior of kill(1): $ kill -l HUP INT QUIT ILL TRAP ABRT EMT FPE KILL BUS SEGV SYS PIPE ALRM TERM URG STOP TSTP CONT CHLD TTIN TTOU IO XCPU XFSZ VTALRM PROF WINCH INFO USR1 USR2 PWR ERR $ ktruss -i -o /tmp/1.x kill -l HUP INT QUITILL TRAPABRTEMT FPE KILL BUS SEGVSYS PIPEALRMTERMURG STOPTSTP CONTCHLDTTINTTOUIO XCPUXFSZVTALRM PROF WINCH INFOUSR1USR2PWR RT0 RT1 RT2 RT3 RT4 RT5 RT6 RT7 RT8 RT9 RT10RT11RT12 RT13RT14RT15RT16RT17RT18RT19RT20RT21 RT22RT23RT24RT25RT26RT27RT28RT29RT30 (gdb) r Starting program: /bin/kill -l HUP INT QUITILL TRAPABRTEMT FPE KILL BUS SEGVSYS PIPEALRMTERMURG STOPTSTP CONTCHLDTTINTTOUIO XCPUXFSZVTALRM PROF WINCH INFOUSR1USR2PWR RT0 RT1 RT2 RT3 RT4 RT5 RT6 RT7 RT8 RT9 RT10RT11RT12 RT13RT14RT15RT16RT17RT18RT19RT20RT21 RT22RT23RT24RT25RT26RT27RT28RT29RT30 [Inferior 1 (process 6257) exited normally] (gdb) What happened to RT signal names? I'm not sure what's wrong as this code works under a debugger. Sanitizers don't catch anything (at least when sanitizing kill.c).
Re: [stos, again] Re: CVS commit: src/sys/arch/amd64
On 04.06.2020 23:41, Andrew Doran wrote: > On Thu, Jun 04, 2020 at 02:35:17AM +0200, Kamil Rytarowski wrote: > >> On 04.06.2020 00:42, Andrew Doran wrote: >>> On Wed, Jun 03, 2020 at 02:03:22AM +0200, Kamil Rytarowski wrote: >>> >>>> On 03.06.2020 01:49, Andrew Doran wrote: >>>>> On the assembly thing recall that recently you expressed a desire to >>>>> remove >>>>> all of the amd64 assembly string functions from libc because of >>>>> sanitizers - >>>>> I invested my time to do up a little demo to try and show you why that's >>>>> not >>>>> a good idea: >>>>> >>>>> http://mail-index.netbsd.org/port-amd64/2020/04/19/msg003226.html >>>> >>>> Please note that interceptors for string functions are not just some >>>> extra burden, but also very useful approach to feedback a fuzzer through >>>> additional coverage. >>>> >>>> At least libFuzzer and honggfuzz wrap many kinds of string functions and >>>> use it for fuzzing. We should add a special mode in KCOV to feedback >>>> userland (syzkaller) with traces from string functions. >>>> >>>> https://github.com/google/honggfuzz/blob/bbb476eec95ad927d6d7d3d367d2b3e38eed3569/libhfuzz/memorycmp.c#L24 >>> >>> No argument from me there at all. I think that's a great idea and was >>> looking at the interceptors in TSAN recently to see how they work. >>> >>> Andrew >>> >> >> My note was not about switching away from ASM functions for certain >> functions, but rather giving an option to disable them under a sanitizer >> and adding an interceptor that can be useful for feedbacking a fuzzer. >> It's still not clear whether we will create such interface in KCOV as it >> has to be coordinated with Google+Linux as we would like to have a >> compatible interface for syzkaller. >> >> TSAN - do you mean the userland ones? > > Right, the userland one. > Great. We used to pass around 95% of upstream LLVM TSan tests. The remaining ones were hard and I had no time to look into them. >> BTW. There is a work-in-progress MKSANITIZER support for TSan, but it >> used to create unkillable processes (kernel bug). Basically when using a >> TSanitized userland applications, you will quickly end up with such >> processes (from AFAIR calling ls(1) and other simple applications are >> enough). >> >> If you are interested, I can share a reproducer. > > Yes please. Is the setup difficult? I plan to look at some of the > remaining issues noted on syzbot over the next while too. > ./build.sh -j8 -N0 -U -u -V MAKECONF=/dev/null -V MKCOMPAT=no -V MKDEBUGLIB=yes -V MKDEBUG=yes -V MKSANITIZER=yes -V USE_SANITIZER=thread -V MKLLVM=yes -V MKGCC=no -V HAVE_LLVM=yes -O /public/netbsd.fuzzer distribution Null mount /dev /dev/pts /tmp and chroot into it. Try to run some basic applications and see creation unkillable processes. Unless that was fixed by an accident/indirectly, you will quickly reproduce it. > Cheers, > Andrew >
Re: [stos, again] Re: CVS commit: src/sys/arch/amd64
On 04.06.2020 00:42, Andrew Doran wrote: > On Wed, Jun 03, 2020 at 02:03:22AM +0200, Kamil Rytarowski wrote: > >> On 03.06.2020 01:49, Andrew Doran wrote: >>> On the assembly thing recall that recently you expressed a desire to remove >>> all of the amd64 assembly string functions from libc because of sanitizers - >>> I invested my time to do up a little demo to try and show you why that's not >>> a good idea: >>> >>> http://mail-index.netbsd.org/port-amd64/2020/04/19/msg003226.html >> >> Please note that interceptors for string functions are not just some >> extra burden, but also very useful approach to feedback a fuzzer through >> additional coverage. >> >> At least libFuzzer and honggfuzz wrap many kinds of string functions and >> use it for fuzzing. We should add a special mode in KCOV to feedback >> userland (syzkaller) with traces from string functions. >> >> https://github.com/google/honggfuzz/blob/bbb476eec95ad927d6d7d3d367d2b3e38eed3569/libhfuzz/memorycmp.c#L24 > > No argument from me there at all. I think that's a great idea and was > looking at the interceptors in TSAN recently to see how they work. > > Andrew > My note was not about switching away from ASM functions for certain functions, but rather giving an option to disable them under a sanitizer and adding an interceptor that can be useful for feedbacking a fuzzer. It's still not clear whether we will create such interface in KCOV as it has to be coordinated with Google+Linux as we would like to have a compatible interface for syzkaller. TSAN - do you mean the userland ones? BTW. There is a work-in-progress MKSANITIZER support for TSan, but it used to create unkillable processes (kernel bug). Basically when using a TSanitized userland applications, you will quickly end up with such processes (from AFAIR calling ls(1) and other simple applications are enough). If you are interested, I can share a reproducer.
Re: [stos, again] Re: CVS commit: src/sys/arch/amd64
On 03.06.2020 01:49, Andrew Doran wrote: > On the assembly thing recall that recently you expressed a desire to remove > all of the amd64 assembly string functions from libc because of sanitizers - > I invested my time to do up a little demo to try and show you why that's not > a good idea: > > http://mail-index.netbsd.org/port-amd64/2020/04/19/msg003226.html Please note that interceptors for string functions are not just some extra burden, but also very useful approach to feedback a fuzzer through additional coverage. At least libFuzzer and honggfuzz wrap many kinds of string functions and use it for fuzzing. We should add a special mode in KCOV to feedback userland (syzkaller) with traces from string functions. https://github.com/google/honggfuzz/blob/bbb476eec95ad927d6d7d3d367d2b3e38eed3569/libhfuzz/memorycmp.c#L24
Re: CVS commit: src/sys/ddb
On 01.06.2020 02:31, Rin Okuyama wrote: > On 2020/06/01 9:23, Kamil Rytarowski wrote: >> I wrote a tiny malloc (libc-style) implementation over a small static >> storage (could be over stack or preallocated on the heap) without any >> external dependencies. >> >> Would it be useful for you? > > At the moment, we need buffers only in db_show_callout(), and static > buffers are enough there, IMO. > > However, in future, if we want to add new features that require dynamic > buffer allocation, we need to implement exclusive allocator for DDB. > > Thanks, > rin If you can find a use-case today in DDB for an exclusive allocator. I have it ready and tested. I intended to use it in ddb for one project, but that project is still unfinished. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/ddb
I wrote a tiny malloc (libc-style) implementation over a small static
storage (could be over stack or preallocated on the heap) without any
external dependencies.
Would it be useful for you?
On 01.06.2020 01:34, Rin Okuyama wrote:
> Module Name: src
> Committed By: rin
> Date: Sun May 31 23:34:34 UTC 2020
>
> Modified Files:
> src/sys/ddb: db_kernel.c
>
> Log Message:
> Switch from kmem_intr_alloc(sz, KM_NOSLEEP) to kmem_alloc(sz, KM_SLEEP).
>
> Clearly document these functions are *not* for DDB session, but for
> permanent data storage when initializing DDB.
>
>
> To generate a diff of this commit:
> cvs rdiff -u -r1.3 -r1.4 src/sys/ddb/db_kernel.c
>
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
>
>
> Modified files:
>
> Index: src/sys/ddb/db_kernel.c
> diff -u src/sys/ddb/db_kernel.c:1.3 src/sys/ddb/db_kernel.c:1.4
> --- src/sys/ddb/db_kernel.c:1.3 Sun May 31 09:42:46 2020
> +++ src/sys/ddb/db_kernel.c Sun May 31 23:34:34 2020
> @@ -1,4 +1,4 @@
> -/* $NetBSD: db_kernel.c,v 1.3 2020/05/31 09:42:46 rin Exp $*/
> +/* $NetBSD: db_kernel.c,v 1.4 2020/05/31 23:34:34 rin Exp $*/
>
> /*-
> * Copyright (c) 2009 The NetBSD Foundation, Inc.
> @@ -30,7 +30,7 @@
> */
>
> #include
> -__KERNEL_RCSID(0, "$NetBSD: db_kernel.c,v 1.3 2020/05/31 09:42:46 rin Exp
> $");
> +__KERNEL_RCSID(0, "$NetBSD: db_kernel.c,v 1.4 2020/05/31 23:34:34 rin Exp
> $");
>
> #include
> #include
> @@ -39,28 +39,33 @@ __KERNEL_RCSID(0, "$NetBSD: db_kernel.c,
>
> /*
> * XXX
> - * DDB can be running in the interrupt context, e.g., when activated from
> - * console. Therefore, we use kmem_intr_alloc(9) and friends here to avoid
> - * assertion failure.
> + * These routines are *not* intended for a DDB session:
> + *
> + * - It disturbes on-going debugged kernel datastructures.
> + *
> + * - DDB can be running in the interrupt context, e.g., when activated from
> + * console. This results in assertion failures in the allocator.
> + *
> + * Use these only for permanent data storage when initializing DDB.
> */
>
> void *
> db_alloc(size_t sz)
> {
>
> - return kmem_intr_alloc(sz, KM_NOSLEEP);
> + return kmem_alloc(sz, KM_SLEEP);
> }
>
> void *
> db_zalloc(size_t sz)
> {
>
> - return kmem_intr_zalloc(sz, KM_NOSLEEP);
> + return kmem_zalloc(sz, KM_SLEEP);
> }
>
> void
> db_free(void *p, size_t sz)
> {
>
> - kmem_intr_free(p, sz);
> + kmem_free(p, sz);
> }
>
signature.asc
Description: OpenPGP digital signature
Re: CVS commit: src/external/bsd/ntp/bin/ntpd
On 29.05.2020 16:15, Tobias Nygren wrote: > On Fri, 29 May 2020 16:08:30 +0200 > Joerg Sonnenberger wrote: > >> On Fri, May 29, 2020 at 10:53:02AM +, Kamil Rytarowski wrote: >>> Module Name:src >>> Committed By: kamil >>> Date: Fri May 29 10:53:02 UTC 2020 >>> >>> Modified Files: >>> src/external/bsd/ntp/bin/ntpd: Makefile >>> >>> Log Message: >>> Fix the ntpd build with Clang/LLVM >>> >>> Set -Wno-format-nonliteral for ntp_refclock.c >> >> Please fix this properly by adding the appropiate format string >> annotation. > > I have these in my local tree. Can't reach cvs. > Feel free to commit them if appropriate. > I will do it once the CVS server will be back. > Index: external/bsd/ntp/dist/libntp/timexsup.c > === > RCS file: /cvsroot/src/external/bsd/ntp/dist/libntp/timexsup.c,v > retrieving revision 1.2 > diff -p -u -r1.2 timexsup.c > --- external/bsd/ntp/dist/libntp/timexsup.c 25 May 2020 20:47:24 - > 1.2 > +++ external/bsd/ntp/dist/libntp/timexsup.c 29 May 2020 14:13:35 - > @@ -30,8 +30,8 @@ clamp_rounded( > dval = floor(dval + 0.5); > > /* clamp / saturate */ > - if (dval >= LONG_MAX) > - return LONG_MAX; > + if (dval >= (double)LONG_MAX) > + return (double)LONG_MAX; > if (dval <= LONG_MIN) > return LONG_MIN; > return (long)dval; > Index: external/bsd/ntp/dist/ntpd/ntp_refclock.c > === > RCS file: /cvsroot/src/external/bsd/ntp/dist/ntpd/ntp_refclock.c,v > retrieving revision 1.13 > diff -p -u -r1.13 ntp_refclock.c > --- external/bsd/ntp/dist/ntpd/ntp_refclock.c 25 May 2020 20:47:25 - > 1.13 > +++ external/bsd/ntp/dist/ntpd/ntp_refclock.c 29 May 2020 14:13:37 - > @@ -1737,7 +1737,7 @@ refclock_save_lcode( > } > > /* format data into a_lastcode */ > -void > +void __printflike(2, 0) > refclock_vformat_lcode( > struct refclockproc * pp, > char const *fmt, > @@ -1757,7 +1757,7 @@ refclock_vformat_lcode( > /* !note! the NUL byte is needed in case vsnprintf() really fails */ > } > > -void > +void __printflike(2, 0) > refclock_format_lcode( > struct refclockproc * pp, > char const *fmt, > signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/tests/rump/modautoload
I will test this with ASan and report back!
On 16.05.2020 16:15, Christos Zoulas wrote:
> That is a completely different issue here. There are no linker tricks.
> We want the module loader to include all the symbols any module
> can require, this is why we load all the libraries.
>
> While it is questionable if nofifofs is part of the base system or not,
> this is the way it was before. Anyway it is easy enough to have it
> both ways. If we ever grow a test that needs the real fifo stuff in
> an autoloaded module, we can deal with that then.
>
> christos
>
>> On May 16, 2020, at 9:46 AM, Kamil Rytarowski wrote:
>>
>> Signed PGP part
>> On 16.05.2020 14:54, Christos Zoulas wrote:
>>> Module Name:src
>>> Committed By: christos
>>> Date: Sat May 16 12:54:27 UTC 2020
>>>
>>> Modified Files:
>>> src/tests/rump/modautoload: Makefile
>>>
>>> Log Message:
>>> Do the same thing with linker flags instead of directly specifying the
>>> archives.
>>>
>>>
>>
>> Is there chance to rename the fifo symbols instead of using linker tricks?
>>
>> I'm also not entirely sure that this will be compatible with sanitizers
>> (and C++ with the ODR rule) at this point.
>>
>>> To generate a diff of this commit:
>>> cvs rdiff -u -r1.10 -r1.11 src/tests/rump/modautoload/Makefile
>>>
>>> Please note that diffs are not public domain; they are subject to the
>>> copyright notices on the relevant files.
>>>
>>>
>>> Modified files:
>>>
>>> Index: src/tests/rump/modautoload/Makefile
>>> diff -u src/tests/rump/modautoload/Makefile:1.10
>>> src/tests/rump/modautoload/Makefile:1.11
>>> --- src/tests/rump/modautoload/Makefile:1.10Sat May 16 08:44:42 2020
>>> +++ src/tests/rump/modautoload/Makefile Sat May 16 08:54:27 2020
>>> @@ -1,4 +1,4 @@
>>> -# $NetBSD: Makefile,v 1.10 2020/05/16 12:44:42 christos Exp $
>>> +# $NetBSD: Makefile,v 1.11 2020/05/16 12:54:27 christos Exp $
>>> #
>>>
>>> .include
>>> @@ -15,11 +15,9 @@ SRCS.t_modautoload+= t_modautoload.c
>>> # subdirectory -- otherwise the LDADD lines would get a little hairy.
>>> LDFLAGS+= -Wl,-E
>>> LDADD+= \
>>> --Wl,--whole-archive \
>>> -${DESTDIR}/usr/lib/librumpvfs_nofifofs.a \
>>> -${DESTDIR}/usr/lib/librumpvfs.a \
>>> -${DESTDIR}/usr/lib/librump.a \
>>> --Wl,--no-whole-archive
>>> +-Wl,--whole-archive -Wl,-Bstatic \
>>> + -lrumpvfs_nofifofs -lrumpvfs -lrump \
>>> +-Wl,-Bdynamic -Wl,--no-whole-archive
>>>
>>> LDADD+= -lrumpuser -lpthread
>>> DPADD+= ${LIBRUMPVFS} ${LIBRUMP} ${LIBRUMPUSER}
>>>
>>
>>
>>
>>
>
signature.asc
Description: OpenPGP digital signature
Re: CVS commit: src/tests/rump/modautoload
On 16.05.2020 14:54, Christos Zoulas wrote:
> Module Name: src
> Committed By: christos
> Date: Sat May 16 12:54:27 UTC 2020
>
> Modified Files:
> src/tests/rump/modautoload: Makefile
>
> Log Message:
> Do the same thing with linker flags instead of directly specifying the
> archives.
>
>
Is there chance to rename the fifo symbols instead of using linker tricks?
I'm also not entirely sure that this will be compatible with sanitizers
(and C++ with the ODR rule) at this point.
> To generate a diff of this commit:
> cvs rdiff -u -r1.10 -r1.11 src/tests/rump/modautoload/Makefile
>
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
>
>
> Modified files:
>
> Index: src/tests/rump/modautoload/Makefile
> diff -u src/tests/rump/modautoload/Makefile:1.10
> src/tests/rump/modautoload/Makefile:1.11
> --- src/tests/rump/modautoload/Makefile:1.10 Sat May 16 08:44:42 2020
> +++ src/tests/rump/modautoload/Makefile Sat May 16 08:54:27 2020
> @@ -1,4 +1,4 @@
> -#$NetBSD: Makefile,v 1.10 2020/05/16 12:44:42 christos Exp $
> +#$NetBSD: Makefile,v 1.11 2020/05/16 12:54:27 christos Exp $
> #
>
> .include
> @@ -15,11 +15,9 @@ SRCS.t_modautoload+= t_modautoload.c
> # subdirectory -- otherwise the LDADD lines would get a little hairy.
> LDFLAGS+=-Wl,-E
> LDADD+= \
> --Wl,--whole-archive \
> -${DESTDIR}/usr/lib/librumpvfs_nofifofs.a \
> -${DESTDIR}/usr/lib/librumpvfs.a \
> -${DESTDIR}/usr/lib/librump.a \
> --Wl,--no-whole-archive
> +-Wl,--whole-archive -Wl,-Bstatic \
> + -lrumpvfs_nofifofs -lrumpvfs -lrump \
> +-Wl,-Bdynamic -Wl,--no-whole-archive
>
> LDADD+= -lrumpuser -lpthread
> DPADD+= ${LIBRUMPVFS} ${LIBRUMP} ${LIBRUMPUSER}
>
signature.asc
Description: OpenPGP digital signature
Re: CVS commit: src/tests/lib/libc/sys
On 15.05.2020 00:43, Taylor R Campbell wrote: >> Date: Thu, 14 May 2020 23:36:28 +0200 >> From: Kamil Rytarowski >> >> If a signal would not reach the child process (as it is ignored or >> masked+SA_IGNOREd) and it is not a crash signal, it is dropped. As I >> checked, it's the design in UNIX to overlook SIGCHLD signals in UNIX. >> Race free signals could be maybe possible, but with some design rethinking. > > I don't understand -- are you saying that if I mask SIGCHLD, e.g. with > sigprocmask(SIG_BLOCK), then because sigprop[SIGCHLD] has SA_IGNORE > set, any SIGCHLD signals will be discarded while I have it masked? > That's it. But it will be discarded only when there is no SIGCHLD signal handler installed. That's the case in the test. A debugger catches regular signals only (except crash related ones) when they reach the debuggee, > This can't be right, so either I misunderstood what you're saying, or > something else is afoot with the test that is making it flaky, or > there's a bug in the kernel. > It's a design. If we install a signal handler for SIGCHLD in the traced the test is very stable and we note SIGCHLD always, so there is no bug. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/tests/lib/libc/sys
On 14.05.2020 23:02, Joerg Sonnenberger wrote: > On Thu, May 14, 2020 at 07:21:35PM +0000, Kamil Rytarowski wrote: >> Module Name: src >> Committed By:kamil >> Date:Thu May 14 19:21:35 UTC 2020 >> >> Modified Files: >> src/tests/lib/libc/sys: t_ptrace_fork_wait.h >> >> Log Message: >> Ignore interception of the SIGCHLD signals. >> >> SIGCHLD once blocked is discarded by the kernel as it has the >> SA_IGNORE property. During the fork(2) operation all signals can be >> shortly blocked and missed (unless there is a registered signal >> handler in the traced child). This leads to a race in this test if >> there would be an intention to catch SIGCHLD. > > This doesn't sound right. sigprocmask and pthread_sigmask shouldn't > affect whether the signal handler is called, only when. Temporarily > masking a signal is different from setting it to SIG_IGN. > > Joerg > I was looking into it and as there is no signal handler for SIGCHLD, the signal is discarded. Another approach to fix the race would be to: 1. Add SIGCHLD signal handler in the traced child. 2. Explicitly masking SIGCHLD in the traced child. If a signal would not reach the child process (as it is ignored or masked+SA_IGNOREd) and it is not a crash signal, it is dropped. As I checked, it's the design in UNIX to overlook SIGCHLD signals in UNIX. Race free signals could be maybe possible, but with some design rethinking. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src
On 12.05.2020 02:59, Joerg Sonnenberger wrote: > On Mon, May 11, 2020 at 11:07:02PM +0200, Kamil Rytarowski wrote: >> On 19.04.2020 03:06, Joerg Sonnenberger wrote: >>> Module Name:src >>> Committed By: joerg >>> Date: Sun Apr 19 01:06:16 UTC 2020 >>> >>> Modified Files: >>> src/lib/libc/gen: pthread_atfork.c >>> src/libexec/ld.elf_so: rtld.c rtld.h symbols.map >>> >>> Log Message: >>> Rename __atomic_fork to __locked_fork and give it &errno as argument. >>> rtld and libc use different storage, so the initial version would >>> incorrectly report the failure reason for fork(). >>> >>> There is still a small race condition inside ld.elf_so as it doesn't use >>> thread-safe errno internally, but that's a more contained internal >>> issue. >>> >>> >> >> >> Should we add the same logic for clone(2)? > > clone only exists for Linux compat. I see no reason to support any fork > emulation for it. > > Joerg > This Linux compat is on the source code level and inside the kernel clone() shares the same code with fork(). clone(2) is a regular syscall available in the NetBSD native ABI syscall layers. There are also users (I have got one). All problems for fork() can be reproduced for clone(). But if we want to just mitigate some issues of fork() users and fix/ignore promptly clone() ones, it is fine. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src
On 19.04.2020 03:06, Joerg Sonnenberger wrote: > Module Name: src > Committed By: joerg > Date: Sun Apr 19 01:06:16 UTC 2020 > > Modified Files: > src/lib/libc/gen: pthread_atfork.c > src/libexec/ld.elf_so: rtld.c rtld.h symbols.map > > Log Message: > Rename __atomic_fork to __locked_fork and give it &errno as argument. > rtld and libc use different storage, so the initial version would > incorrectly report the failure reason for fork(). > > There is still a small race condition inside ld.elf_so as it doesn't use > thread-safe errno internally, but that's a more contained internal > issue. > > Should we add the same logic for clone(2)? signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/tests/lib/libc/sys
On 11.05.2020 13:35, Robert Elz wrote: > Date:Mon, 11 May 2020 11:03:15 + > From: "Kamil Rytarowski" > Message-ID: <2020050315.54b13f...@cvs.netbsd.org> > > | Do not fail when trying to kill a dying process > | > | A dying process can disappear for a while. Rather than aborting, retry > | sending SIGKILL to it. > > I don't understand this ... a process should never be able to > disappear and then reappear (not in any way). If a SIGKILL (or > ptrace(PT_KILL) fails with a "no such process" error, then repeating > it won't (or shouldn't) help - if it does, there's a kernel bug that > needs fixing (and it is OK for the test to fail until that happens.) > > Further, if the reason for this failure is that the process is > dying, you probably never needed the kill in the first place (and > no, I don't mean it should be deleted - the parent is unlikely to > know the state of the child, so killing it, if that is what is needed > is the right thing to do ... just that if the kill fails because you > were too late issuing it, it isn't an error, just a race that you lost, > and certainly shouldn't be repeated). > > But more than that, adding an infinite loop to the test, where you keep > doing the kill forever until it succeeds, or errno somehow stops being > ESRCH looks like a recipe for disaster. > > Just do the kill once, ignore the error if it is ESRCH (and probably > also ECHILD) report other errors as failures. > > kre > The only purpose of the test is to check whether misaligned program counter can crash the kernel (it can for NetBSD/sparc). Later, if a process dies or runs is not important, thus it is being killed. A process can disappear after dying and before reappearing as a zombie. This is not a bug, but a predicted race. We already discussed it in the past, whether to return the same process multiple times or overlook it for a while during the transition dying->zombie. Once an entity died it disappeared so the same is true for a process. Doing the kill once (and missing the process) is still possibly enough, but correcting it with SIGKILL does not cost. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/bin/kill
On 06.05.2020 14:48, Robert Elz wrote: > Date:Wed, 6 May 2020 11:28:42 +0200 > From: Kamil Rytarowski > Message-ID: > > > | While there, we have got a long standing issue with wait.1 man page, > > Huh! I had no idea any such thing existed... (do you know of any > more bizarre ones like that?) > I don't know other cases. > | it should be either removed (as the wait(1) program is gone) or adapted > | with the reality of being a builtin. > > Yes, it should.Was there ever a wait(1) program? POSIX says there > should be (along with cd umask ulimit ...) but the general feeling here > is that that's just plain stupid... (and I agree). > > > I'm not going to right now, as I'm not sure which is the right path > to take - there has been (in the past) some discussion about making > man pages for all of the sh builtins (so one doesn't need to know the > trick of how to find their doc in the sh(1) manpage easily - no idea how > it is done with csh as I stopped using that decades ago). > > If we decide to do that, then fixing that page to be rational would > be the right thing to do, if not, then deleting it. > > I'll see if I can find out what the likely outcome of a discussion of > that will be (hopefully avoiding actually needing the discission again). > If there's a resolution, I will make it happen. > Possibly, wait.1 shall clearly note that this is a shell builtin and it would be enough. cd.1 notes this indirectly. > kre > > ps: this one is not quite so important as the librumpuser issue... > And wrt that, I am not (I hope) a total cretin ... I would not have > objected if you had removed the (now) useless variable along with that > one line of code... (but what you did is fine). > > signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/kern
On 07.05.2020 07:46, Michael van Elst wrote: > On Wed, May 06, 2020 at 12:39:21PM +0200, Kamil Rytarowski wrote: > > Hi Kamil, > >> If I revert the pipe(2) changes on top of NetBSD-current, the test is no >> longer racy again. > > I tried 9.99.60 with and without my bugfix. The test always failed > after some time with exactly that error. > > If the change really had an effect, there would be a use-after-free bug > somewhere else. > > > Greetings, > Thanks, I will investigating further. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys/kern
This caused regressions in t_ptrace_wait* tests and random
hangs/timeouts/failures.
If I revert the pipe(2) changes on top of NetBSD-current, the test is no
longer racy again.
http://netbsd.org/~kamil/patch-00249-pipe-revert.txt
Reproducer:
cd /usr/tests/lib/libc/sys
./t_ptrace_waitpid tracer_sysctl_lookup_without_duplicates
It fails in a non-deterministic number of iterations:
[ 126.7088900] sorry, pid 20803 was killed: orphaned traced process
failed: /usr/src/tests/lib/libc/sys/t_ptrace_topology_wait.h:191:
msg_read_child("tracer ready" " from parent " "parent_tracer",
&parent_tracer, &msg, sizeof(msg)) == 0: Undefined error: 0
With this patch it is easier to reproduce the race:
Index: t_ptrace_topology_wait.h
===
RCS file: /cvsroot/src/tests/lib/libc/sys/t_ptrace_topology_wait.h,v
retrieving revision 1.1
diff -u -r1.1 t_ptrace_topology_wait.h
--- t_ptrace_topology_wait.h5 May 2020 00:33:37 - 1.1
+++ t_ptrace_topology_wait.h6 May 2020 10:32:37 -
@@ -248,7 +248,7 @@
ATF_TC(tracer_sysctl_lookup_without_duplicates);
ATF_TC_HEAD(tracer_sysctl_lookup_without_duplicates, tc)
{
- atf_tc_set_md_var(tc, "timeout", "15");
+// atf_tc_set_md_var(tc, "timeout", "15");
atf_tc_set_md_var(tc, "descr",
"Assert that await_zombie() in attach1 always finds a single "
"process and no other error is reported");
@@ -269,11 +269,13 @@
start = time(NULL);
while (true) {
DPRINTF("Step: %lu\n", N);
+ if (N % 100 == 0)
+ printf("Step: %lu\n", N);
tracer_sees_terminaton_before_the_parent_raw(true, false,
false);
end = time(NULL);
diff = difftime(end, start);
- if (diff >= 5.0)
+ if (diff >= 30.0)
break;
++N;
}
Can you have a look?
On 26.04.2019 19:20, Michael van Elst wrote:
> Module Name: src
> Committed By: mlelstv
> Date: Fri Apr 26 17:20:49 UTC 2019
>
> Modified Files:
> src/sys/kern: sys_pipe.c
>
> Log Message:
> Clean up pipe structure before recycling it.
>
>
> To generate a diff of this commit:
> cvs rdiff -u -r1.146 -r1.147 src/sys/kern/sys_pipe.c
>
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
>
>
> Modified files:
>
> Index: src/sys/kern/sys_pipe.c
> diff -u src/sys/kern/sys_pipe.c:1.146 src/sys/kern/sys_pipe.c:1.147
> --- src/sys/kern/sys_pipe.c:1.146 Sun Jun 10 17:54:51 2018
> +++ src/sys/kern/sys_pipe.c Fri Apr 26 17:20:49 2019
> @@ -1,4 +1,4 @@
> -/* $NetBSD: sys_pipe.c,v 1.146 2018/06/10 17:54:51 jdolecek Exp $ */
> +/* $NetBSD: sys_pipe.c,v 1.147 2019/04/26 17:20:49 mlelstv Exp $ */
>
> /*-
> * Copyright (c) 2003, 2007, 2008, 2009 The NetBSD Foundation, Inc.
> @@ -68,7 +68,7 @@
> */
>
> #include
> -__KERNEL_RCSID(0, "$NetBSD: sys_pipe.c,v 1.146 2018/06/10 17:54:51 jdolecek
> Exp $");
> +__KERNEL_RCSID(0, "$NetBSD: sys_pipe.c,v 1.147 2019/04/26 17:20:49 mlelstv
> Exp $");
>
> #include
> #include
> @@ -1331,6 +1331,8 @@ pipeclose(struct pipe *pipe)
> free_resources:
> pipe->pipe_pgid = 0;
> pipe->pipe_state = PIPE_SIGNALR;
> + pipe->pipe_peer = NULL;
> + pipe->pipe_lock = NULL;
> pipe_free_kmem(pipe);
> if (pipe->pipe_kmem != 0) {
> pool_cache_put(pipe_rd_cache, pipe);
>
signature.asc
Description: OpenPGP digital signature
Re: CVS commit: src/bin/kill
On 06.05.2020 11:07, Robert Elz wrote: > Module Name: src > Committed By: kre > Date: Wed May 6 09:07:15 UTC 2020 > > Modified Files: > src/bin/kill: kill.1 > > Log Message: > kill is built-in to more than just csh(1). > While here, add missing Xr sh 1 (which was previously needed, moreso now) > and also include STOP and CONT in the list of common signals. > > Thanks for taking care of this. While there, we have got a long standing issue with wait.1 man page, it should be either removed (as the wait(1) program is gone) or adapted with the reality of being a builtin. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/lib/librumpuser
On 06.05.2020 07:17, Alistair Crooks wrote: > > > On Tue, 5 May 2020 at 20:54, Kamil Rytarowski <mailto:n...@gmx.com>> wrote: > > Ping? We are blocked by this in GSoC now. > > > I doubt that you are "blocked by this in GSoC", as the GSoC projects > were just announced yesterday. > > You should be planning the milestones right now with your student We already worked on preparation to this GSoC in the past few months. I reverted my fix from Match with a promise to see a better fix by kre@ but it never happened. This bug breaks rumpkernel usage with ASan (and possibly MSan/TSan which weren't tested with rump). Lack of fix blocks two selected GSoC tasks (or forces us to keep local patches). signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/lib/librumpuser
Ping? We are blocked by this in GSoC now. On 01.04.2020 20:19, Kamil Rytarowski wrote: > On 01.04.2020 17:06, Robert Elz wrote: >> Date:Wed, 1 Apr 2020 15:54:15 +0200 >> From:Kamil Rytarowski >> Message-ID: <969362d2-d93e-2cf4-7437-ab593ab11...@gmx.com> >> >> | Ping? This still breaks. >> >> I am still working on it.Best I can tell at the minute is that the \0 >> is potentially needed (in a theoretical sense) but not by anything >> operating rationally. >> >> That is, when rump is used the strings will already always be \0 terminated >> and the extra one added (the one that is off the end of the array) is never >> needed, there's always an earlier one. >> >> However, the relevant struct (that contains the string) comes from some >> other process, and while if that process is running rump code, which is >> what is intended to happen, all will be OK (I believe, I am not finished >> checking all of that code), if it is something else, generating rump >> packets, and passing them through, then we have no idea what will >> be there, and the \0 termination cannot be guaranteed (and if we don't >> do something, the rump process will eventually do bizarre things, that >> out of the array \0 is currently preventing that possibility). >> >> I see two reasonable paths forward here: >> >> 1. instead of adding the \0 off the end of the array, check that the >> array is already \0 terminated (it should be, and always is in the ATF >> test uses of rump - I ran the tests with a check in place, and it never >> failed) - the \0 is always in the final byte of the array (the one you >> overwrote in your earlier change, which meant that the changed line was >> just a no-op, in practice, as suspected earlier.) >> >> 2. When we are reading an exec rump struct, allocate (and zero - the zero >> part is already present) 1 byte more than will be received from the >> sending process, so that the final byte will always remain as a \0, and >> we will absolutely guarantee that the string will be \0 terminated (in >> all normal cases it would end up terminated by two \0's). >> >> If we do either of these, we don't need to waste time verifying that rump >> always does send (in every case) a \0 terminated string (digging through the >> code to work out where some of these structs get built is a slow process) >> as the actual problem will be solved either way. >> >> Solution 1 makes it an error, and the rup process will fail the exec if >> the path isn't correctly \0 terminated. Solution 2 does what the code >> currently does (effectively) adding a \0 beyond the string that is received >> from the sending process, but does it within the array bounds (by making the >> array bigger) rather than outside them. >> >> Opinions for which is better? >> > > Going for 2. is a little bit safer and we can reduce researching corner > cases that might never happen anyway. > >> kre >> > signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/sys
On 24.04.2020 05:22, Jason R Thorpe wrote: > Module Name: src > Committed By: thorpej > Date: Fri Apr 24 03:22:06 UTC 2020 > > Modified Files: > src/sys/compat/linux/common: linux_exec.c linux_sched.c > src/sys/kern: kern_exec.c kern_exit.c kern_fork.c kern_lwp.c > kern_proc.c sys_lwp.c > src/sys/sys: lwp.h proc.h > > Log Message: > Overhaul the way LWP IDs are allocated. Instead of each LWP having it's > own LWP ID space, LWP IDs came from the same number space as PIDs. The > lead LWP of a process gets the PID as its LID. If a multi-LWP process's > lead LWP exits, the PID persists for the process. > > In addition to providing system-wide unique thread IDs, this also lets us > eliminate the per-process LWP radix tree, and some associated locks. > > Remove the separate "global thread ID" map added previously; it is no longer > needed to provide this functionality. > > Nudged in this direction by ad@ and chs@. > This is a good idea (and preexisting in other kernels), unfortunately we had locking issues in rust. If a multithreaded process is forked, we shall create a replica of a calling thread and keep mutexes functional. We tried to preserve the caller's LWP to guarantee this. This problem can be back. I don't ask to revert or revisit this change, but if it will be back, we shall find a solution for it. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/tests/lib/libc/sys
On 24.04.2020 05:25, Jason R Thorpe wrote: > Module Name: src > Committed By: thorpej > Date: Fri Apr 24 03:25:20 UTC 2020 > > Modified Files: > src/tests/lib/libc/sys: t_ptrace_wait.c t_ptrace_x86_wait.h > > Log Message: > Update for new LWP behavior -- as of 9.99.59, the LWP ID of a single-LWP > process is the PID, not 1. > Thanks. These tests shall not rely on specific LWP numbers and I will remove the asserts. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src
On 04.04.2020 22:20, Jason R Thorpe wrote:
> Module Name: src
> Committed By: thorpej
> Date: Sat Apr 4 20:20:12 UTC 2020
>
> Modified Files:
> src/sys/compat/netbsd32: syscalls.master
> src/sys/kern: kern_exit.c kern_lwp.c sys_lwp.c syscalls.master
> src/sys/sys: lwp.h
> Added Files:
> src/tests/lib/libc/sys: t_lwp_tid.c
>
> Log Message:
> Add support for lazily generating a "global thread ID" for a LWP. This
> identifier uniquely identifies an LWP across the entire system, and will
> be used in future improvements in user-space synchronization primitives.
>
> (Test disabled and libc stub not included intentionally so as to avoid
> multiple libc version bumps.)
>
> --- src/sys/sys/lwp.h:1.204 Sat Apr 4 06:51:46 2020
> +++ src/sys/sys/lwp.h Sat Apr 4 20:20:12 2020
> @@ -1,4 +1,4 @@
> -/* $NetBSD: lwp.h,v 1.204 2020/04/04 06:51:46 maxv Exp $ */
> +/* $NetBSD: lwp.h,v 1.205 2020/04/04 20:20:12 thorpej Exp $*/
>
> /*
> * Copyright (c) 2001, 2006, 2007, 2008, 2009, 2010, 2019, 2020
> @@ -135,6 +135,24 @@ struct lwp {
> u_int l_slptime; /* l: time since last blocked */
> booll_vforkwaiting; /* a: vfork() waiting */
>
> + /* User-space synchronization. */
> + uintptr_t l___reserved; /* reserved for future use */
> + /*
> + * The global thread ID has special locking and access
> + * considerations. Because many LWPs may never need one,
> + * global thread IDs are allocated lazily in lwp_gettid().
> + * l___tid is not bean to be accessed directly unless
> + * the accessor has specific knowledge that doing so
> + * is safe. l___tid is only assigned by the LWP itself.
> + * Once assigned, it is stable until the LWP exits.
> + * An LWP assigns its own thread ID unlocked before it
> + * reaches visibility to the rest of the system, and
> + * can access its own thread ID unlocked. But once
> + * published, it must hold the proc's lock to change
> + * the value.
> + */
> + lwpid_t l___tid;/* p: global thread id */
> +
Would it be possible to keep a global unique TID as 64-bit integer
(int64_t) equal to: pid << 32 | lwpid ?
That way we could have predicatable numbers in the system and possibly
simplify the involved code avoiding one extra unique 32-bit id.
Are there any (atomic?) shortcomings of this approach?
signature.asc
Description: OpenPGP digital signature
Re: CVS commit: src/include
On 17.04.2020 22:22, Joerg Sonnenberger wrote: > On Fri, Apr 17, 2020 at 03:22:35PM +0000, Kamil Rytarowski wrote: >> Module Name: src >> Committed By:kamil >> Date:Fri Apr 17 15:22:35 UTC 2020 >> >> Modified Files: >> src/include: assert.h >> >> Log Message: >> Remove the static_assert() fallback for pre-C11 and pre-C++11 >> >> C++ without real static_assert() can be incompatible with the C fallback >> as presented in openjdk. >> >> A pre-C11 compiler can be picky on the implementation. > > So did you actually test that the tree compiles with this? Just asking, > since your own ptrace tests depend on static_assert... > > Joerg > I will fix it! signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/tools/binutils
On 04.04.2020 02:47, Christos Zoulas wrote: > Module Name: src > Committed By: christos > Date: Sat Apr 4 00:47:52 UTC 2020 > > Modified Files: > src/tools/binutils: mknative-binutils > > Log Message: > Handle libctf new in binutils 2.34 > > Please note that the upstreamed libctf version was not (at least when I last looked into it) compatible with what we get for CTF from other distfiles.
Re: CVS commit: src/usr.bin/kdump
On 02.04.2020 23:35, Christos Zoulas wrote:
> Ah debugging remnants. I'll remove it. Can you look at PR/55128?
>
OK!
> static inline union savefpu *
> fpu_lwp_area(struct lwp *l)
> {
> struct pcb *pcb = lwp_getpcb(l);
> union savefpu *area = &pcb->pcb_savefpu;
>
> KASSERT((l->l_flag & LW_SYSTEM) == 0);
> if (l == curlwp) {
> fpu_save();
> }
> KASSERT(!(l->l_md.md_flags & MDL_FPU_IN_CPU)); <- this will fire if
> the debugger calls it with l != curlew and it uses cpu
>
> return area;
> }
>
>
>> On Apr 2, 2020, at 5:31 PM, Kamil Rytarowski wrote:
>>
>> On 02.04.2020 19:40, Christos Zoulas wrote:
>>> +set -x
>>> +AWK=gawk
>>> +
>>
>> gawk?
>
Re: CVS commit: src/usr.bin/kdump
On 02.04.2020 19:40, Christos Zoulas wrote: > +set -x > +AWK=gawk > + gawk?
Re: CVS commit: src/lib/librumpuser
On 01.04.2020 17:06, Robert Elz wrote: > Date:Wed, 1 Apr 2020 15:54:15 +0200 > From: Kamil Rytarowski > Message-ID: <969362d2-d93e-2cf4-7437-ab593ab11...@gmx.com> > > | Ping? This still breaks. > > I am still working on it.Best I can tell at the minute is that the \0 > is potentially needed (in a theoretical sense) but not by anything > operating rationally. > > That is, when rump is used the strings will already always be \0 terminated > and the extra one added (the one that is off the end of the array) is never > needed, there's always an earlier one. > > However, the relevant struct (that contains the string) comes from some > other process, and while if that process is running rump code, which is > what is intended to happen, all will be OK (I believe, I am not finished > checking all of that code), if it is something else, generating rump > packets, and passing them through, then we have no idea what will > be there, and the \0 termination cannot be guaranteed (and if we don't > do something, the rump process will eventually do bizarre things, that > out of the array \0 is currently preventing that possibility). > > I see two reasonable paths forward here: > > 1. instead of adding the \0 off the end of the array, check that the > array is already \0 terminated (it should be, and always is in the ATF > test uses of rump - I ran the tests with a check in place, and it never > failed) - the \0 is always in the final byte of the array (the one you > overwrote in your earlier change, which meant that the changed line was > just a no-op, in practice, as suspected earlier.) > > 2. When we are reading an exec rump struct, allocate (and zero - the zero > part is already present) 1 byte more than will be received from the > sending process, so that the final byte will always remain as a \0, and > we will absolutely guarantee that the string will be \0 terminated (in > all normal cases it would end up terminated by two \0's). > > If we do either of these, we don't need to waste time verifying that rump > always does send (in every case) a \0 terminated string (digging through the > code to work out where some of these structs get built is a slow process) > as the actual problem will be solved either way. > > Solution 1 makes it an error, and the rup process will fail the exec if > the path isn't correctly \0 terminated. Solution 2 does what the code > currently does (effectively) adding a \0 beyond the string that is received > from the sending process, but does it within the array bounds (by making the > array bigger) rather than outside them. > > Opinions for which is better? > Going for 2. is a little bit safer and we can reduce researching corner cases that might never happen anyway. > kre >
Re: CVS commit: src/sys/modules/examples/fopsmapper
On 01.04.2020 16:17, Christos Zoulas wrote: > In article , > Paul Goyette wrote: >> On Wed, 1 Apr 2020, Kamil Rytarowski wrote: >> >>> On 01.04.2020 15:47, Robert Elz wrote: >>>> Date:Wed, 1 Apr 2020 11:45:53 + >>>> From:"Kamil Rytarowski" >>>> Message-ID: <20200401114554.05167f...@cvs.netbsd.org> >>>> >>>> | Log Message: >>>> | Avoid comparison between signed and unsigned integer >>>> | >>>> | Cast PAGE_SIZE to size_t. >>>> >>>> This kind of pedantry is going way too far, PAGE_SIZE is a compile >>>> time constant (1 << PAGE_SHIFT) which is an int (and so signed, >>>> nominally) but one which is known to be positive. >>>> >>> >>> I got reports that certain ports no longer build due to: >>> >>> src/sys/modules/examples/fopsmapper/fopsmapper.c:118:11: error: >>> comparison between signed and unsigned integer expressions >>> [-Werror=sign-compare] >>> if (size != PAGE_SIZE) >>> ^~ >>> cc1: all warnings being treated as errors >> >> >> There's a lot of modules that fail for this with WARNS=5 when being >> built as loadable modules. >> >> That's why so many of the individual module Makefiles have explicit >> WARNS=4. (It seems that when modules are built-in as part of the >> kernel, they are by default built with WARNS=4.) > > Which we have been slowly fixing. I think in this case the sign-compare > warnings are annoying, but putting casts on each warning is cluttering > the code needlessly. Unfortunately the alternative (to make the PAGESIZE > constant unsigned is more dangerous -- unless of course you are willing to > examine all the places it is used and make sure that the semantics don't > change). Another way would be to make: > > #define PAGESIZE_U ((unsigned)PAGESIZE) > > and use that instead; eventually when all instances of PAGESIZE have been > converted to PAGESIZE_U it can be removed. But is it worth it? There are > perhaps better things to do. But anyway you want to proceed should be > discussed in tech-kern. Adding piecemeal casts everywhere does not make > the world a better place. > > christos > Does it look better? http://netbsd.org/~kamil/patch-00244-fopsmapper-PAGE_SIZE.txt Perhaps we could switch PAGE_SIZE to unsigned.. but it is too much for now.
Re: CVS commit: src/sys/modules/examples/fopsmapper
On 01.04.2020 16:10, Paul Goyette wrote: > On Wed, 1 Apr 2020, Kamil Rytarowski wrote: > >> On 01.04.2020 15:47, Robert Elz wrote: >>> Date: Wed, 1 Apr 2020 11:45:53 + >>> From: "Kamil Rytarowski" >>> Message-ID: <20200401114554.05167f...@cvs.netbsd.org> >>> >>> | Log Message: >>> | Avoid comparison between signed and unsigned integer >>> | >>> | Cast PAGE_SIZE to size_t. >>> >>> This kind of pedantry is going way too far, PAGE_SIZE is a compile >>> time constant (1 << PAGE_SHIFT) which is an int (and so signed, >>> nominally) but one which is known to be positive. >>> >> >> I got reports that certain ports no longer build due to: >> >> src/sys/modules/examples/fopsmapper/fopsmapper.c:118:11: error: >> comparison between signed and unsigned integer expressions >> [-Werror=sign-compare] >> if (size != PAGE_SIZE) >> ^~ >> cc1: all warnings being treated as errors > > > There's a lot of modules that fail for this with WARNS=5 when being > built as loadable modules. > > That's why so many of the individual module Makefiles have explicit > WARNS=4. (It seems that when modules are built-in as part of the > kernel, they are by default built with WARNS=4.) > I tracked it to WARNS=3 # Rather than our usual WARNS=5, we need to use 3, since there are a # lot of signed-vs-unsigned compares WARNS= 3 Personally I have got no preference. I'm fine with an explicit cast as it is in a single place only needed (hopefully).
Re: CVS commit: src/lib/librumpuser
On 24.03.2020 19:37, Kamil Rytarowski wrote: > On 24.03.2020 15:41, Robert Elz wrote: >> Date:Tue, 24 Mar 2020 13:27:45 +0100 >> From:Kamil Rytarowski >> Message-ID: <5ec1195a-f1c8-cd46-6a14-ea29da109...@gmx.com> >> >> | I patched it myself only when I reproduced the problems myself. >> >> I have no doubt that there's a bug that needs fixing - it is the fix >> proposed that is wrong. My guess is that most probably it is simply >> doing nothing useful (no harm, no good either) but I need to confirm >> that. If it is, the correct fix is simply to delete the line (both >> times it was changed). If not, there's a more serious problem elsewhere >> that needs fixing elsewhere (after which the line can be deleted!) >> >> | OK. I will do it and please fix it in a better way. >> >> Working on it now. >> > > Thanks. > >> I haven't seen the revert yet, when I do I will commit the fix (or a fix, >> this one is somewhat debatble what is correct, though what is there now >> is obviously wrong ... but just like the one in question, while wrong, it >> is, in practice, harmless, at least in any normal use of librump). >> >> The fix for this issue needs to wait until the real problem the offending >> line is there to deal with (if any, which I suspect is not the case) is >> found. >> > > ASan detects not a hypothetical, but factual momory corruption. > > I'm attaching a report from ASan. > > = > ==11092==ERROR: AddressSanitizer: heap-buffer-overflow on address > 0x6020101e at pc 0x7f7ff6a10419 bp 0x7f7fe942fcb0 sp 0x7f7fe942fca8 > WRITE of size 1 at 0x6020101e thread T30 > #0 0x7f7ff6a10418 in handlereq > /usr/src/lib/librumpuser/rumpuser_sp.c:984:18 > #1 0x7f7ff6a10418 in spserver > /usr/src/lib/librumpuser/rumpuser_sp.c:1280:7 > #2 0x7f7ff660cf36 in pthread__create_tramp > /usr/src/lib/libpthread/pthread.c:587:11 > > 0x6020101e is located 0 bytes to the right of 14-byte region > [0x60201010,0x6020101e) > allocated by thread T30 here: > #0 0x311793 in malloc (/usr/bin/rump_server+0x111793) > #1 0x7f7ff6a0e5bb in readframe > /usr/src/lib/librumpuser/sp_common.c:505:18 > #2 0x7f7ff6a0e5bb in spserver > /usr/src/lib/librumpuser/rumpuser_sp.c:1268:13 > #3 0x7f7ff660cf36 in pthread__create_tramp > /usr/src/lib/libpthread/pthread.c:587:11 > > Thread T30 created by T0 here: > #0 0x2e054d in pthread_create (/usr/bin/rump_server+0xe054d) > > SUMMARY: AddressSanitizer: heap-buffer-overflow > /usr/src/lib/librumpuser/rumpuser_sp.c:984:18 in handlereq > Shadow bytes around the buggy address: > 0x4c0401b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > 0x4c0401c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > 0x4c0401d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > 0x4c0401e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > 0x4c0401f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > =>0x4c040200: fa fa 00[06]fa fa fa fa fa fa fa fa fa fa fa fa > 0x4c040210: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > 0x4c040220: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > 0x4c040230: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > 0x4c040240: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > 0x4c040250: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa > Shadow byte legend (one shadow byte represents 8 application bytes): > Addressable: 00 > Partially addressable: 01 02 03 04 05 06 07 > Heap left redzone: fa > Freed heap region: fd > Stack left redzone: f1 > Stack mid redzone: f2 > Stack right redzone: f3 > Stack after return: f5 > Stack use after scope: f8 > Global redzone: f9 > Global init order: f6 > Poisoned by user:f7 > Container overflow: fc > Array cookie:ac > Intra object redzone:bb > ASan internal: fe > Left alloca redzone: ca > Right alloca redzone:cb > Shadow gap: cc > ==11092==ABORTING > > Getting now more debug info is too time consuming (build times are > excessive in my setup). > Ping? This still breaks.
Re: CVS commit: src/sys/modules/examples/fopsmapper
On 01.04.2020 15:47, Robert Elz wrote: > Date:Wed, 1 Apr 2020 11:45:53 + > From: "Kamil Rytarowski" > Message-ID: <20200401114554.05167f...@cvs.netbsd.org> > > | Log Message: > | Avoid comparison between signed and unsigned integer > | > | Cast PAGE_SIZE to size_t. > > This kind of pedantry is going way too far, PAGE_SIZE is a compile > time constant (1 << PAGE_SHIFT) which is an int (and so signed, > nominally) but one which is known to be positive. > I got reports that certain ports no longer build due to: src/sys/modules/examples/fopsmapper/fopsmapper.c:118:11: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare] if (size != PAGE_SIZE) ^~ cc1: all warnings being treated as errors
Re: CVS commit: src/external/gpl3
On 28.03.2020 05:39, matthew green wrote: >> Date:Sat, 28 Mar 2020 11:46:29 +1100 >> From:matthew green >> Message-ID: <15233.1585356...@splode.eterna.com.au> >> >> | can we just leave this as-is and let netbsd GCC people care? >> >> Only if the GCC people do care, and understand the issue, and >> implement what we want > note i said "netbsd GCC people". i mean me, specifically. The problem is that libiberty is a generic gnu library wrapping software. I landed onto it from GDB. I can see now that P_tmpdir was changed last year to /tmp. OK. So the issue is understood. The problem will solve itself with some time. Meanwhile we could backport it to older branches and report to Darwin people. commit 69f10cfa0cf51ad935e9513474315231717749b0 Author: mrg Date: Fri Dec 13 20:25:16 2019 + move P_tmpdir from "/var/tmp/" to "/tmp/". the main effect of this is to make GCC and other libiberty using tools use /tmp instead of /var/tmp for compiler temp files, which can be a bottleneck on larger systems. a survey of other platforms shows only OSX also uses /var/tmp, everyone else has switched to /tmp long ago. cons: some smaller systems may have a smaller /tmp than /var/tmp, and this may cause builds to fail with out of space earlier. point the build to /var/tmp using TMPDIR in this case. one can argue that setting TMPDIR would work around this, but we want to have the effect for all users without having special setup.
Re: CVS commit: src/external/gpl3
On 27.03.2020 18:42, Martin Husemann wrote: > On Fri, Mar 27, 2020 at 06:36:29PM +0100, Kamil Rytarowski wrote: >> I tested the code without our local patch as mentioned in my commit, >> even in the snapshot 1 commit before jak@'s change. >> >> In all the cases I get P_tmpdir defined and pointing to "/tmp/". >> >> I have not tested it on NetBSD-8 as host. All my tests were with NetBSD >> 9.99.51 as host with GCC style distribution. > > We do not care that much for -8 (even not enough to have that patch you are > backing out pulled up). I only used it to test as that was what I had at hand > w/o the "force /tmp" commit. > If this words out of the box with P_tmpnam on newer NetBSD it might be a good enough reason to backout the patch. > If libiberty has been fixed upstream and the fix is not needed any more: > great! > I will try to research why I get P_tmpnam defined. > From your description it was not clear to me if you did a native gcc build > (./configure && make or something) or used our in-tree reachover > infrastructure > (which might be what was causing the bug). > I tested all GCC/GDB build modes: - in pkgsrc - standalone (./configure && make) - ./build.sh tools - ./build.sh generated distribution In all of them P_tmpdir worked for me. > Martin >
Re: CVS commit: src/external/gpl3
On 27.03.2020 18:24, Martin Husemann wrote: > On Fri, Mar 27, 2020 at 06:14:24PM +0100, Kamil Rytarowski wrote: >> On 27.03.2020 16:01, Martin Husemann wrote: >>> Which compiler version did you test? >>> >> >> I tested on NetBSD HEAD GCC style distribution as host. > > Not sure I understand. You need to test the in-tree build with the change > backed out, but maybe that is what you did? > > Martin > I tested the code without our local patch as mentioned in my commit, even in the snapshot 1 commit before jak@'s change. In all the cases I get P_tmpdir defined and pointing to "/tmp/". I have not tested it on NetBSD-8 as host. All my tests were with NetBSD 9.99.51 as host with GCC style distribution.
Re: CVS commit: src/external/gpl3
On 27.03.2020 16:01, Martin Husemann wrote:
> Which compiler version did you test?
>
I tested on NetBSD HEAD GCC style distribution as host.
$ uname -rms
NetBSD 9.99.51 amd64
> I see this on netbsd-8:
>
>> echo $TMPDIR
> TMPDIR: Undefined variable.
>> ktrace -i $TOOLDIR/bin/x86_64--netbsd-gcc base64.c
> [..]
>> kdump | fgrep tmp
> [..]
> 19418 1 collect2 NAMI "/var/tmp//ccs9D5nv.le"
> 19418 1 collect2 NAMI "/var/tmp//ccs9D5nv.le"
> 8963 1 x86_64--netbsd-g NAMI "/var/tmp//ccrQ0mcM.res"
> 8963 1 x86_64--netbsd-g NAMI "/var/tmp//ccrQ0mcM.res"
> [..]
>
I can see:
20138 1 x86_64--netbsd-g __stat50("/tmp/", 0x7f7fce80) = 0
20138 1 x86_64--netbsd-g open("/tmp//ccIpnVEp.o", 0xa02, 0x180) =
14929 1 collect2 open("/tmp//ccrQbdnE.o", 0xa02, 0x180) = 3
14929 1 collect2 __stat50("/tmp/", 0x7f7fd8b0) = 0
14929 1 collect2 open("/tmp//ccHF5RS9.ld", 0xa02, 0x180) = 3
6756 1 ld open("/tmp//ccIpnVEp.o", 0, 0x1b6) = 4
6756 1 ld open("/tmp//ccIpnVEp.o", 0, 0x7f7ff7e59ea0) = 5
>
> Martin
>
Re: CVS commit: src/external/gpl3
I've found out that this patch in not needed, at least on NetBSD and
likely Linux.
The algorithm to pick tmpdir is as follows:
- check for memoized tmpdir
- try env variables
- try P_tmpdir
- try /var/tmp, then /usr/tmp, then /tmp
- fallback to current dir ('.')
- save memoized tmpdir
P_tmpdir is defined as /tmp on NetBSD for XOPEN/NETBSD SOURCE.
I've verifed that P_tmpdir is always picked and defined (*) in
make-temp-file.c for all of our GNU toolchain programs (gdb, gcc,
binutils) in tools build, in distribution build, in pkgsrc build and in
standalone build.
This means that we return choose_tmpdir() always "/tmp" (**)
I've double checked this with the source code 1 commit before this
change (***) and in the current sources.
So, the question is... is this just a hack for build on Darwin or some
other OS without P_tmpdir? Was this patch ever really needed?
If this is just a hack for one specific OS, I recommend to remove our
local modification and rely on TMPDIR in such setup or discuss with
upstream GCC (libiberty) a patch how to point it to /tmp.
(*) I've checked the claim with:
#ifdef P_tmpdir
/* We really want a directory name here as if concatenated with
say \dir
we do not end up with a double \\ which defines an UNC path. */
if (strcmp (P_tmpdir, "\\") == 0)
base = try_dir ("\\.", base);
else
base = try_dir (P_tmpdir, base);
#else
#error 1
#endif
(**) To be extra sure, I have verified this with logging to a temporary
file:
memoized_tmpdir = tmpdir;
+
+ {
+FILE *fp = fopen("/tmp/log.tmp.txt", "a");
+fprintf(fp, "PID=%d PROG='%s' TMPDIR='%s'\n", getpid(),
getprogname(), tmpdir);
+fclose(fp);
+ }
$ LC_ALL=C grep -c /var/tmp /tmp/log.tmp.txt
0
$ LC_ALL=C grep -c tmp /tmp/log.tmp.txt
48745
$ head /tmp/log.tmp.txt
PID=16262 PROG='x86_64--netbsd-gcc' TMPDIR='/tmp//'
PID=20242 PROG='x86_64--netbsd-gcc' TMPDIR='/tmp//'
PID=18851 PROG='x86_64--netbsd-gcc' TMPDIR='/tmp//'
PID=11095 PROG='x86_64--netbsd-gcc' TMPDIR='/tmp//'
PID=8036 PROG='x86_64--netbsd-gcc' TMPDIR='/tmp//'
PID=26914 PROG='x86_64--netbsd-gcc' TMPDIR='/tmp//'
PID=8930 PROG='x86_64--netbsd-gcc' TMPDIR='/tmp//'
PID=21615 PROG='x86_64--netbsd-gcc' TMPDIR='/tmp//'
PID=27201 PROG='x86_64--netbsd-gcc' TMPDIR='/tmp//'
PID=11745 PROG='x86_64--netbsd-gcc' TMPDIR='/tmp//'
(***)
commit 0eba0e605b50d99665de27a77f5ca870f2c8b41d (HEAD -> test)
Author: ozaki-r
Date: Mon Aug 10 09:32:01 2015 +
Fix head and cleanup definitions for cache_expiration tests
On 26.03.2020 01:43, Jonathan A. Kollasch wrote:
> We (to the extent I can assert that as an individual developer) insist
> on keeping it.
>
> Jonathan Kollasch
>
> On Thu, Mar 26, 2020 at 01:26:05AM +0100, Kamil Rytarowski wrote:
>> Upstream (GCC) is strongly against this change (even under __NetBSD__
>> ifdef) as /var/tmp is typically larger than /tmp:
>>
>>> I'd strongly recommend against this as-is.
>>>
>>> The whole reason we prefer /var/tmp is because it's often dramatically
>> larger
>>> than a ram-backed /tmp.
>>
>> -- by Jeff Law.
>>
>> Do we insist on this patch? Can we remove it from local sources?
>>
>> The same effect can be achieved with env(1) variables: TMP=/tmp
>> TEMPDIR=/tmp or TEMP=/tmp.
>>
>> On 10.08.2015 17:45, Jonathan A. Kollasch wrote:
>>> Module Name:src
>>> Committed By: jakllsch
>>> Date: Mon Aug 10 15:45:40 UTC 2015
>>>
>>> Modified Files:
>>> src/external/gpl3/binutils/dist/libiberty: make-temp-file.c
>>> src/external/gpl3/gcc/dist/libiberty: make-temp-file.c
>>> src/external/gpl3/gdb/dist/libiberty: make-temp-file.c
>>>
>>> Log Message:
>>> Correct temporary directory preference order in libiberty's choose_tmpdir().
>>>
>>> Because it is intended to be persistent, /var/tmp is about the worst
>>> possible
>>> choice for temporary files for most users of libiberty. /tmp works better,
>>> because the the defined semantics of /tmp allow for a non-persistent tmpfs
>>> to be used. This should improve performance when /tmp is a tmpfs and it is
>>> difficult or impossible to have an environment variable or command line
>>> -pipe
>>> flag passed to every piece of the toolchain.
>>>
>>>
>>> To generate a diff of this commit:
>>> cvs rdiff -u -r1.1.1.3 -r1.
Re: CVS commit: src/external/gpl3
On 26.03.2020 17:49, Robert Elz wrote: > Date:Thu, 26 Mar 2020 16:28:18 +0100 > From: Kamil Rytarowski > Message-ID: <84460ebb-b4bf-f3ee-e51b-e27d0b6e2...@gmx.com> > > > | That is negligible cost of getting TMPDIR propagated to most programs. > > Sure, it isn't much, for one program, but when you're running tens of > thousands, it all adds up. > It's still negligible for hundreds of millions calls. Modern CPUs like Ryzen Threadripper 3990X can execute that extra amount of instructions (around 600) in a single clock cycle. If we want to optimize exec(), there would be better places to do so. But nonetheless, I'm trying to fix it un GCC without TMPDIR.
Re: CVS commit: src/external/gpl3
On 26.03.2020 17:03, Kamil Rytarowski wrote: > On 26.03.2020 16:17, Greg Troxel wrote: >> I don't think we can go from "upstream is unwilling to do X" >> to "impose pain on NetBSD" by making "divergence bad" be the >> highest-weighted concern. We have to say "what is the minimally >> painful way to cope". > > I agree here that we want to get /tmp by default, but in real life we > lost these local modifications anyway. None of the pkgsrc packages ships > with this and upstream GCC knows better.. so I keep trying to find a > creative way to get it done right. > > Patching every libiberty copy is almost not doable (we don't patch any > user of it in pkgsrc). > > If I will fail to convince GCC to add a way to tune this tmpdir, we will > live with these libiberty patches in src. > > If GCC will be unconvinced, I don't plan to remove this local modification. > > On 26.03.2020 16:56, Taylor R Campbell wrote: >> We don't need to set TMPDIR=/tmp anywhere because that's already what >> correct programs use by default (for example, via _PATH_TMP). > > Good idea. I will try to reuse _PATH_TMP and submit upstream. > OK, I have got another idea how to address it. Thank you for the feedback.
Re: CVS commit: src/external/gpl3
On 26.03.2020 16:17, Greg Troxel wrote: > I don't think we can go from "upstream is unwilling to do X" > to "impose pain on NetBSD" by making "divergence bad" be the > highest-weighted concern. We have to say "what is the minimally > painful way to cope". I agree here that we want to get /tmp by default, but in real life we lost these local modifications anyway. None of the pkgsrc packages ships with this and upstream GCC knows better.. so I keep trying to find a creative way to get it done right. Patching every libiberty copy is almost not doable (we don't patch any user of it in pkgsrc). If I will fail to convince GCC to add a way to tune this tmpdir, we will live with these libiberty patches in src. If GCC will be unconvinced, I don't plan to remove this local modification. On 26.03.2020 16:56, Taylor R Campbell wrote: > We don't need to set TMPDIR=/tmp anywhere because that's already what > correct programs use by default (for example, via _PATH_TMP). Good idea. I will try to reuse _PATH_TMP and submit upstream. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/external/gpl3
On 26.03.2020 16:02, Robert Elz wrote: > EVery extra env var has a cost in every exec of absolutely everything. As this is true.. but as I have benchmarked it. For true(1) on amd64, we execute 0.05% more instructions for extra TMPDIR environment variable. $ ./singlestepper true Total count: 1286742 $ TMPDIR=/tmp ./singlestepper true Total count: 1287399 That is negligible cost of getting TMPDIR propagated to most programs. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/external/gpl3
On 26.03.2020 15:52, Greg Troxel wrote: > What is the real problem here? I think it's great that NetBSD-specific > fixes are being upstreamed, and that we are reducing gratuitous changes > from upstream. But upstream's position that the default tmp should be > /var/tmp is at odds with our and traditional norms, and really seems to > just not make sense. (Perhaps it does make sense on typical Linux, and > perhaps upstream should have OS-specific tmp defaults.) Adding > complexity to NetBSD config files and users for the sake of reducing a > diff seems like a bad tradeoff. Generally speaking divergence with upstream is a problem and we lost these changes anyway in pkgsrc and every standalone GNU toolchain copy. Finding a creative way to make this at least tunable is challenging with the GCC people. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/external/gpl3
On 26.03.2020 15:01, Martin Husemann wrote: > On Thu, Mar 26, 2020 at 02:57:40PM +0100, Kamil Rytarowski wrote: >> The build of tools could be fixed independently. > The problem is that we build the whole system with the tools gcc, and that > gcc misbehaves (or so I understood). > > So pointing TMPDIR anywhere does not help. > Right. Addressing tools build independently (honoring TMPDIR?) and defining TMPDIR in /etc shell profile for common use inside the distribution. > Martin > signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/external/gpl3
On 26.03.2020 09:48, Martin Husemann wrote: > On Thu, Mar 26, 2020 at 12:51:24AM +, Taylor R Campbell wrote: >> We should keep the change. There is no semantic justification for >> putting build-time temporary files in the directory for temporary >> files that are meant to persist across reboot. These temporary files >> _cannot_ be used if interrupted -- let alone by a reboot. > > The original bug is somewhere else though: it seems (for reasons not well > understood) that the NetBSD tools build of gcc (contrary to the installed > one and all native build gcc) does *NOT* honour $TMPDIR in env. (Somthing > broken in the ifdef maze in libberty?) > > We should fix that. My local build scripts (and the build cluster) properly > sets TMPDIR, so if that worked, the default would not matter. > Maybe we could specify TMPDIR somewhere in /etc and point to /tmp? The build of tools could be fixed independently. > Martin > signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/external/gpl3
Upstream (GCC) is strongly against this change (even under __NetBSD__ ifdef) as /var/tmp is typically larger than /tmp: > I'd strongly recommend against this as-is. > > The whole reason we prefer /var/tmp is because it's often dramatically larger > than a ram-backed /tmp. -- by Jeff Law. Do we insist on this patch? Can we remove it from local sources? The same effect can be achieved with env(1) variables: TMP=/tmp TEMPDIR=/tmp or TEMP=/tmp. On 10.08.2015 17:45, Jonathan A. Kollasch wrote: > Module Name: src > Committed By: jakllsch > Date: Mon Aug 10 15:45:40 UTC 2015 > > Modified Files: > src/external/gpl3/binutils/dist/libiberty: make-temp-file.c > src/external/gpl3/gcc/dist/libiberty: make-temp-file.c > src/external/gpl3/gdb/dist/libiberty: make-temp-file.c > > Log Message: > Correct temporary directory preference order in libiberty's choose_tmpdir(). > > Because it is intended to be persistent, /var/tmp is about the worst possible > choice for temporary files for most users of libiberty. /tmp works better, > because the the defined semantics of /tmp allow for a non-persistent tmpfs > to be used. This should improve performance when /tmp is a tmpfs and it is > difficult or impossible to have an environment variable or command line -pipe > flag passed to every piece of the toolchain. > > > To generate a diff of this commit: > cvs rdiff -u -r1.1.1.3 -r1.2 \ > src/external/gpl3/binutils/dist/libiberty/make-temp-file.c > cvs rdiff -u -r1.1.1.2 -r1.2 \ > src/external/gpl3/gcc/dist/libiberty/make-temp-file.c > cvs rdiff -u -r1.1.1.1 -r1.2 \ > src/external/gpl3/gdb/dist/libiberty/make-temp-file.c > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files. > > > Modified files: > > Index: src/external/gpl3/binutils/dist/libiberty/make-temp-file.c > diff -u src/external/gpl3/binutils/dist/libiberty/make-temp-file.c:1.1.1.3 > src/external/gpl3/binutils/dist/libiberty/make-temp-file.c:1.2 > --- src/external/gpl3/binutils/dist/libiberty/make-temp-file.c:1.1.1.3 > Sun Sep 29 13:46:38 2013 > +++ src/external/gpl3/binutils/dist/libiberty/make-temp-file.cMon Aug > 10 15:45:40 2015 > @@ -130,10 +130,10 @@ choose_tmpdir (void) > base = try_dir (P_tmpdir, base); > #endif > > - /* Try /var/tmp, /usr/tmp, then /tmp. */ > + /* Try /tmp, /var/tmp, then /usr/tmp. */ > + base = try_dir (tmp, base); >base = try_dir (vartmp, base); >base = try_dir (usrtmp, base); > - base = try_dir (tmp, base); > >/* If all else fails, use the current directory! */ >if (base == 0) > > Index: src/external/gpl3/gcc/dist/libiberty/make-temp-file.c > diff -u src/external/gpl3/gcc/dist/libiberty/make-temp-file.c:1.1.1.2 > src/external/gpl3/gcc/dist/libiberty/make-temp-file.c:1.2 > --- src/external/gpl3/gcc/dist/libiberty/make-temp-file.c:1.1.1.2 Sat Mar > 1 08:41:40 2014 > +++ src/external/gpl3/gcc/dist/libiberty/make-temp-file.c Mon Aug 10 > 15:45:40 2015 > @@ -130,10 +130,10 @@ choose_tmpdir (void) > base = try_dir (P_tmpdir, base); > #endif > > - /* Try /var/tmp, /usr/tmp, then /tmp. */ > + /* Try /tmp, /var/tmp, then /usr/tmp. */ > + base = try_dir (tmp, base); >base = try_dir (vartmp, base); >base = try_dir (usrtmp, base); > - base = try_dir (tmp, base); > >/* If all else fails, use the current directory! */ >if (base == 0) > > Index: src/external/gpl3/gdb/dist/libiberty/make-temp-file.c > diff -u src/external/gpl3/gdb/dist/libiberty/make-temp-file.c:1.1.1.1 > src/external/gpl3/gdb/dist/libiberty/make-temp-file.c:1.2 > --- src/external/gpl3/gdb/dist/libiberty/make-temp-file.c:1.1.1.1 Sat Sep > 24 19:49:55 2011 > +++ src/external/gpl3/gdb/dist/libiberty/make-temp-file.c Mon Aug 10 > 15:45:40 2015 > @@ -130,10 +130,10 @@ choose_tmpdir (void) > base = try_dir (P_tmpdir, base); > #endif > > - /* Try /var/tmp, /usr/tmp, then /tmp. */ > + /* Try /tmp, /var/tmp, then /usr/tmp. */ > + base = try_dir (tmp, base); >base = try_dir (vartmp, base); >base = try_dir (usrtmp, base); > - base = try_dir (tmp, base); > >/* If all else fails, use the current directory! */ >if (base == 0) > signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/lib/librumpuser
On 24.03.2020 15:41, Robert Elz wrote: > Date:Tue, 24 Mar 2020 13:27:45 +0100 > From: Kamil Rytarowski > Message-ID: <5ec1195a-f1c8-cd46-6a14-ea29da109...@gmx.com> > > | I patched it myself only when I reproduced the problems myself. > > I have no doubt that there's a bug that needs fixing - it is the fix > proposed that is wrong. My guess is that most probably it is simply > doing nothing useful (no harm, no good either) but I need to confirm > that. If it is, the correct fix is simply to delete the line (both > times it was changed). If not, there's a more serious problem elsewhere > that needs fixing elsewhere (after which the line can be deleted!) > > | OK. I will do it and please fix it in a better way. > > Working on it now. > Thanks. > I haven't seen the revert yet, when I do I will commit the fix (or a fix, > this one is somewhat debatble what is correct, though what is there now > is obviously wrong ... but just like the one in question, while wrong, it > is, in practice, harmless, at least in any normal use of librump). > > The fix for this issue needs to wait until the real problem the offending > line is there to deal with (if any, which I suspect is not the case) is > found. > ASan detects not a hypothetical, but factual momory corruption. I'm attaching a report from ASan. = ==11092==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6020101e at pc 0x7f7ff6a10419 bp 0x7f7fe942fcb0 sp 0x7f7fe942fca8 WRITE of size 1 at 0x6020101e thread T30 #0 0x7f7ff6a10418 in handlereq /usr/src/lib/librumpuser/rumpuser_sp.c:984:18 #1 0x7f7ff6a10418 in spserver /usr/src/lib/librumpuser/rumpuser_sp.c:1280:7 #2 0x7f7ff660cf36 in pthread__create_tramp /usr/src/lib/libpthread/pthread.c:587:11 0x6020101e is located 0 bytes to the right of 14-byte region [0x60201010,0x6020101e) allocated by thread T30 here: #0 0x311793 in malloc (/usr/bin/rump_server+0x111793) #1 0x7f7ff6a0e5bb in readframe /usr/src/lib/librumpuser/sp_common.c:505:18 #2 0x7f7ff6a0e5bb in spserver /usr/src/lib/librumpuser/rumpuser_sp.c:1268:13 #3 0x7f7ff660cf36 in pthread__create_tramp /usr/src/lib/libpthread/pthread.c:587:11 Thread T30 created by T0 here: #0 0x2e054d in pthread_create (/usr/bin/rump_server+0xe054d) SUMMARY: AddressSanitizer: heap-buffer-overflow /usr/src/lib/librumpuser/rumpuser_sp.c:984:18 in handlereq Shadow bytes around the buggy address: 0x4c0401b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x4c0401c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x4c0401d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x4c0401e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x4c0401f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa =>0x4c040200: fa fa 00[06]fa fa fa fa fa fa fa fa fa fa fa fa 0x4c040210: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x4c040220: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x4c040230: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x4c040240: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x4c040250: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user:f7 Container overflow: fc Array cookie:ac Intra object redzone:bb ASan internal: fe Left alloca redzone: ca Right alloca redzone:cb Shadow gap: cc ==11092==ABORTING Getting now more debug info is too time consuming (build times are excessive in my setup). signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/lib/librumpuser
On 24.03.2020 06:09, Robert Elz wrote: > Date:Tue, 24 Mar 2020 05:40:13 +0100 > From: Kamil Rytarowski > Message-ID: > > > | This patch was sitting in the tree since August 2019. > > In your tree I assume you mean, it certainly hasn't been in mine. > A similar fix was done by shm@ with his public repo in August last year. https://github.com/LogicalTrust/fuzzrump-src/commit/f5d2fb34ea597bb92a72d31236ad2f350a2fd8be That work was not secret as it was presented in at least 2 public and crowded conference. As far as I recall it was not reported as PR or on a ML before. I patched it myself only when I reproduced the problems myself. > OK, then please revert that change (which cannot be the right fix, regardless > of what the right one really is) and file a PR, so it can be fixed properly. > OK. I will do it and please fix it in a better way. > kre > > ps: while looking at this I spotted a (complely unrelated) bug in the same > file that should be fixed (no, sanitisers will never find this one, but > human reading easily might) - which I will fix, but I'd prefer to do that > after this fix is reverted, just so those two commit messages are adjacent. > Thank you in advance! signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/lib/librumpuser
On 24.03.2020 05:27, Robert Elz wrote: > There was no great urgency to "fix" > this particular one, it doesn't seem to have been causing any real problems, > and could have easily waited a few days (or weeks, or even months) until > the correct solution was found. > > kre > This patch was sitting in the tree since August 2019. This patch was tested to be operational. I am aware that writing out of allocated buffer is for some people considered as 'no bug'. I'm fine to see it fixed in a better way than manually injected \0. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/external/gpl3/gdb/dist/bfd
On 26.02.2016 17:28, Christos Zoulas wrote:
> Module Name: src
> Committed By: christos
> Date: Fri Feb 26 16:28:14 UTC 2016
>
> Modified Files:
> src/external/gpl3/gdb/dist/bfd: merge.c
>
> Log Message:
> CID 420802: Avoid NULL deref.
>
>
> To generate a diff of this commit:
> cvs rdiff -u -r1.1.1.4 -r1.2 src/external/gpl3/gdb/dist/bfd/merge.c
>
> Please note that diffs are not public domain; they are subject to the
> copyright notices on the relevant files.
>
>
> Modified files:
>
> Index: src/external/gpl3/gdb/dist/bfd/merge.c
> diff -u src/external/gpl3/gdb/dist/bfd/merge.c:1.1.1.4
> src/external/gpl3/gdb/dist/bfd/merge.c:1.2
> --- src/external/gpl3/gdb/dist/bfd/merge.c:1.1.1.4Tue Feb 2 22:00:11 2016
> +++ src/external/gpl3/gdb/dist/bfd/merge.cFri Feb 26 11:28:14 2016
> @@ -334,7 +334,7 @@ sec_merge_emit (bfd *abfd, struct sec_me
>
>/* Trailing alignment needed? */
>off = sec->size - off;
> - if (off != 0)
> + if (pad != NULL && off != 0)
> {
>if (contents)
> memcpy (contents + offset, pad, off);
>
It looks to me like a false positive.
pad is checked just after bfd_zmalloc():
pad = (char *) bfd_zmalloc (pad_len);
if (pad == NULL)
return FALSE;
If I am not overlooking something, I will drop this local patch as not
upstreamable.
signature.asc
Description: OpenPGP digital signature
Re: CVS commit: src/sys/rump
On 09.03.2020 15:31, Taylor R Campbell wrote: >> Module Name:src >> Committed By: kamil >> Date: Mon Mar 9 00:03:00 UTC 2020 >> >> Modified Files: >> src/sys/rump: Makefile.rump >> >> Log Message: >> Build RUMP with -fno-delete-null-pointer-checks on all compilers > > I asked you to hold off on making this change, ten hours before you > proceeded to make it: > > https://mail-index.NetBSD.org/tech-kern/2020/03/08/msg026125.html > > It remains unclear whether the change is actually necessary. > It is necessary. The tool detects real UB and the tool is not broken. I went that far that I summoned people from compilers and the C + C++ standard to confirm this. > Please back this out until the discussion has reached a conclusion, > and please don't unilaterally move ahead to make changes that are > still under active discussion -- especially when someone in the > discussion you initiated has specifically asked you to wait. > > If there is a dispute that has deadlocked in public discussion and > requires a resolution, then you can present a case to core and ask for > a ruling. But right now the discussion is not deadlocked and nobody > has asked for core to step in to resolve any dispute in order to make > progress. > I will revert it for the time being on this demand. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/tests/lib/libc/sys
On 07.03.2020 15:53, Christos Zoulas wrote: > Module Name: src > Committed By: christos > Date: Sat Mar 7 14:53:14 UTC 2020 > > Modified Files: > src/tests/lib/libc/sys: t_ptrace_wait.c t_ptrace_wait.h > > Log Message: > Try to fix the build. This is why all those inlines should really be in a > separate file as regular function. The code is too large and hard to manage > this way, and only increases in complexity as time goes by. > > What build configuration was broken? signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/include
On 02.03.2020 03:58, matthew green wrote: >>> On Sun, Mar 01, 2020 at 03:08:16PM +0000, Kamil Rytarowski wrote: >>>> Module Name: src >>>> Committed By: kamil >>>> Date: Sun Mar 1 15:08:16 UTC 2020 >>>> >>>> Modified Files: >>>>src/include: stddef.h >>>> >>>> Log Message: >>>> Expose max_align_t to C99/C++ > > where was this discussed? Yes. > > why are controvesial changes being commited without any > warning or discussion on tech-toolchain? > It was discussed on ML. > this is getting ridiculous. please revert this and the > GNU hashc hanges, and hold off any other changes pending > discussions on public lists. > > GNU hashes are not generated (and so used) on NetBSD. > .mrg. > signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/doc
On 01.03.2020 22:36, Joerg Sonnenberger wrote: > On Sun, Mar 01, 2020 at 10:20:44PM +0100, Kamil Rytarowski wrote: >> If that will nor realize, I will request to enable GNU_HASH for NetBSD-11. > > You have so far demonstrated no justification at all beyond handwaving > for why it should be included in first place. Seriously, stop rushing > code without doing your homework to demonstrate that it is (1) correct > (2) an actual improvement (3) understanding concerns raised in the past. > I have limited ressources and having to deal with crap like the > max_align_t commit just costs a lot of time for no good reason. > (1) As it was tested locally and capable to use in the following benchmark it works. Feel free to double check, catch potential bugs in the DT_GNU_HASH implementation inside RTLD and correct them. (2) I linked to benchmarks in my commit message. I have reproduced the ~50% performance boost claim here: http://netbsd.org/~kamil/ld/DT_HASH-DT_GNU_HASH-benchmark-2020-03-02.txt Feel free to tune the benchmark for other scenarios and write your own tests. (3) DT_HASH is generally considered as too slow with raised requirements on dynamically linked libraries over the past 15 years. >> GNU_HASH is industry standard for all modern Linux and any other >> mainstream BSD distribution already for around a decade. > > If you want an industry standard, please use the GNU/Linux distribution > of your choice. Arguing based on industry standards is a non sequitur. > Our current LD and RTLD feature set matrix is at best said.. conservative in the context of all other BSDs and Linux (possibly Solaris) and undemanding in performance. If NetBSD wants to be relevant for development of heavy C/C++ code, it must not be worse compared to alternatives. I'm working on getting us on and get NetBSD on par with industry standard toolchain support (and LLVM lld here). > Joerg > So. Unless there will be a viable alternative materialized as DT_NETBSD_HASH, I will flip DT_GNU_HASH on in NetBSD-11. If we cannot make nice things on our own locally, we shall rely on 3rd party industry standards. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/include
On 01.03.2020 22:54, Joerg Sonnenberger wrote: > On Sun, Mar 01, 2020 at 10:26:08PM +0100, Kamil Rytarowski wrote: >> On 01.03.2020 19:31, Joerg Sonnenberger wrote: >>> On Sun, Mar 01, 2020 at 03:08:16PM +, Kamil Rytarowski wrote: >>>> Module Name: src >>>> Committed By: kamil >>>> Date: Sun Mar 1 15:08:16 UTC 2020 >>>> >>>> Modified Files: >>>>src/include: stddef.h >>>> >>>> Log Message: >>>> Expose max_align_t to C99/C++ >>> >>> Please revert this immediately. It is just wrong. >>> >> >> Rationale? libc++ does not work and I perceive insisting it as a losing >> battle and and zero gain. > > As you do know, there is a patch for libc++ under review to fix this > properly. It is even generally agreed that the current behavior is a > bug. Your patch broke well defined C++03 programs. > >>>> This problem does not exist on OSs like Linux as they get this namespace >>>> visibility defined inside LLVM or GNU toolchain headers. NetBSD ships with >>>> its own stddef.h, rather than relying on a toolchain and its internal >>>> extensions. >>> >>> This is incorrect as well. >>> >> >> No stddef.h in GLIBC is a matter of fact. > > $ printf '#include \nmax_align_t foo;' | clang++ -std=c++03 -x c++ - > :2:1: error: unknown type name 'max_align_t' > $ printf '#include \nmax_align_t foo;' | g++ -std=c++03 -x c++ - > :2:1: error: ‘max_align_t’ does not name a type > > Joerg > As you know what I am referring to: https://github.com/llvm/llvm-project/commit/03dd205c1516d9930a80101a7e0a6793af47ec9e I will revert this but I don't feel convinced. signature.asc Description: OpenPGP digital signature
Re: CVS commit: src/include
On 01.03.2020 19:31, Joerg Sonnenberger wrote: > On Sun, Mar 01, 2020 at 03:08:16PM +0000, Kamil Rytarowski wrote: >> Module Name: src >> Committed By:kamil >> Date:Sun Mar 1 15:08:16 UTC 2020 >> >> Modified Files: >> src/include: stddef.h >> >> Log Message: >> Expose max_align_t to C99/C++ > > Please revert this immediately. It is just wrong. > Rationale? libc++ does not work and I perceive insisting it as a losing battle and and zero gain. >> This problem does not exist on OSs like Linux as they get this namespace >> visibility defined inside LLVM or GNU toolchain headers. NetBSD ships with >> its own stddef.h, rather than relying on a toolchain and its internal >> extensions. > > This is incorrect as well. > No stddef.h in GLIBC is a matter of fact. > Joerg > signature.asc Description: OpenPGP digital signature
