Re: Anyone has time for a unittesting issue?
On 10/02/2016 09:19 AM, Joakim wrote: On Sunday, 2 October 2016 at 01:30:15 UTC, Vladimir Panteleev wrote: On Sunday, 2 October 2016 at 01:23:18 UTC, Andrei Alexandrescu wrote: There, we list the full content of /tmp, recursively. That's not quite the right thing to do. Not only the run time is arbitrarily slow, but the test may file if certain subdirs are inaccessible. Vladimir, maybe I can convince you to find a better solution for that, too? :o) https://issues.dlang.org/show_bug.cgi?id=16571 The unittest in question is ensuring that compilation succeeds: https://github.com/dlang/phobos/pull/3821 Solution: move the dirEntries call inside the __traits(compiles) check. I'm doing server maintenance tonight so no PR for the moment :) This is partly my fault: https://github.com/dlang/phobos/pull/4332/files#diff-52580fb75b304ba7b04a6b178fe6cdf4L3270 I'll submit a PR with the fix you suggest. Thanks! -- Andrei
Re: Anyone has time for a unittesting issue?
On Sunday, 2 October 2016 at 01:30:15 UTC, Vladimir Panteleev wrote: On Sunday, 2 October 2016 at 01:23:18 UTC, Andrei Alexandrescu wrote: There, we list the full content of /tmp, recursively. That's not quite the right thing to do. Not only the run time is arbitrarily slow, but the test may file if certain subdirs are inaccessible. Vladimir, maybe I can convince you to find a better solution for that, too? :o) https://issues.dlang.org/show_bug.cgi?id=16571 The unittest in question is ensuring that compilation succeeds: https://github.com/dlang/phobos/pull/3821 Solution: move the dirEntries call inside the __traits(compiles) check. I'm doing server maintenance tonight so no PR for the moment :) This is partly my fault: https://github.com/dlang/phobos/pull/4332/files#diff-52580fb75b304ba7b04a6b178fe6cdf4L3270 I'll submit a PR with the fix you suggest.
Re: Anyone has time for a unittesting issue?
On Saturday, October 01, 2016 19:51:05 Dicebot via Digitalmars-d wrote: > I wonder if Phobos provides any > cross-platform way to do so - I remember some PR on topic but in > current documentation there seems to be nothing useful mentioned, We had it, and it got yanked, because there was screaming just because it increased the "hello world" binary by about 500KB because of stuff getting pulled in that really shouldn't be pulled in just to use writeln. There's a bug report for putting it back https://issues.dlang.org/show_bug.cgi?id=14599 but the compiler issues that resulted in way more getting pulled in with writeln than should have been have to be fixed first. - Jonathan M Davis
Re: Anyone has time for a unittesting issue?
On Sunday, 2 October 2016 at 01:23:18 UTC, Andrei Alexandrescu wrote: There, we list the full content of /tmp, recursively. That's not quite the right thing to do. Not only the run time is arbitrarily slow, but the test may file if certain subdirs are inaccessible. Vladimir, maybe I can convince you to find a better solution for that, too? :o) https://issues.dlang.org/show_bug.cgi?id=16571 The unittest in question is ensuring that compilation succeeds: https://github.com/dlang/phobos/pull/3821 Solution: move the dirEntries call inside the __traits(compiles) check. I'm doing server maintenance tonight so no PR for the moment :)
Re: Anyone has time for a unittesting issue?
On 10/01/2016 06:08 PM, Vladimir Panteleev wrote: On Saturday, 1 October 2016 at 21:25:56 UTC, Andrei Alexandrescu wrote: Interesting, thanks. Seems like the most robust thing to do is to not use /tmp/ after all. That will cause performance regressions on systems that mount /tmp in RAM. Subtle point, thanks. On the same topic, I think I found where the intermittent test failures are: https://github.com/dlang/phobos/blob/master/std/file.d#L3312 There, we list the full content of /tmp, recursively. That's not quite the right thing to do. Not only the run time is arbitrarily slow, but the test may file if certain subdirs are inaccessible. Vladimir, maybe I can convince you to find a better solution for that, too? :o) https://issues.dlang.org/show_bug.cgi?id=16571 Thanks, Andrei
Re: Anyone has time for a unittesting issue?
On Saturday, 1 October 2016 at 21:25:56 UTC, Andrei Alexandrescu wrote: Interesting, thanks. Seems like the most robust thing to do is to not use /tmp/ after all. That will cause performance regressions on systems that mount /tmp in RAM.
Re: Anyone has time for a unittesting issue?
On Saturday, 1 October 2016 at 17:35:29 UTC, Andrei Alexandrescu wrote: I think /tmp/ is mounted per user so we should be good. I have never seen this. In fact, I'm not familiar with any mechanisms of "per-user" mounts on POSIX systems. The general practice of creating files in /tmp/ is to either put the UID in the filename, or use unique random filenames (e.g. via mkstemp). If this is security-sensitive, we should not be dismissive about any aspects of this. It would also be nice to have a VERY SIMPLE mechanism to delete old runs (e.g. a day or more). FWIW, systemd mounts /tmp as a tmpfs, and OS X seems to delete files in /tmp older than 3 days.
Re: Anyone has time for a unittesting issue?
On 10/01/2016 05:00 PM, Guillaume Boucher wrote: On Saturday, 1 October 2016 at 19:51:05 UTC, Dicebot wrote: I think that is OK but only if actual file inside the dir is created with `mktemp --tmpdir=/tmp/.dmd-test-run/` (or using a similar technique). This is not sufficient. Any user can create a symlink from /tmp/.dmd-test-run/ to e.g. /very/private/root/directory/ (that user can't access it, but symlinks don't check the permission of the target). Executed as root user, mktemp then creates a unique file in /very/private/root/directory/. Which can be used for example to litter a filesystem, which hurts performance or fills disks. That's why I was saying /tmp/.dmd-test-run/ should have permissions 0700. I think a better naming scheme would be /tmp/dmd-testrun-username/, or if that already exists with wrong permissions /tmp/dmd-testrun-username-RANDOMCHARS/. The files inside that directory don't need to have random names (afaik). Interesting, thanks. Seems like the most robust thing to do is to not use /tmp/ after all. In fact, I've encountered errors because (if I remember correctly) we list the content of the /tmp/ directory in unittests and we get exceptions because some dirs are not accessible. A PR reviewing all uses of /tmp/ would be awesome. Andrei
Re: Anyone has time for a unittesting issue?
On Saturday, 1 October 2016 at 19:51:05 UTC, Dicebot wrote: I think that is OK but only if actual file inside the dir is created with `mktemp --tmpdir=/tmp/.dmd-test-run/` (or using a similar technique). This is not sufficient. Any user can create a symlink from /tmp/.dmd-test-run/ to e.g. /very/private/root/directory/ (that user can't access it, but symlinks don't check the permission of the target). Executed as root user, mktemp then creates a unique file in /very/private/root/directory/. Which can be used for example to litter a filesystem, which hurts performance or fills disks. That's why I was saying /tmp/.dmd-test-run/ should have permissions 0700. I think a better naming scheme would be /tmp/dmd-testrun-username/, or if that already exists with wrong permissions /tmp/dmd-testrun-username-RANDOMCHARS/. The files inside that directory don't need to have random names (afaik). It seems like more practical issue is simply that no regular destruction of /tmp/ happens on your system. I'm not sure what you were implying by this. Deleting anything in /tmp while it's mounted is a very bad idea. The permission-check of /tmp/dmd-testrun-username/ relies on the fact that the directory won't be deleted. If it will, then this introduces a race condition.
Re: Anyone has time for a unittesting issue?
On Saturday, 1 October 2016 at 19:32:08 UTC, Andrei Alexandrescu wrote: Not like this is real security concern in dmd case but guidelines like "don't make /tmp/ path predictable" exist exactly so that one can have simple safe default and not worry about possibilities. This may be a misunderstanding. I'm saying is to switch from unpredictable paths rooted in /tmp/ to equally unpredictable paths rooted in /tmp/.dmd-test-run/. I think that is OK but only if actual file inside the dir is created with `mktemp --tmpdir=/tmp/.dmd-test-run/` (or using a similar technique). I wonder if Phobos provides any cross-platform way to do so - I remember some PR on topic but in current documentation there seems to be nothing useful mentioned,
Re: Anyone has time for a unittesting issue?
On 10/01/2016 03:29 PM, Dicebot wrote: On Saturday, 1 October 2016 at 18:24:07 UTC, Andrei Alexandrescu wrote: Granted, no contest. Seems to me we could be a better denizen of said junkyard. What I noticed other apps do is create one directory in /tmp and then place their junk in there. -- Andrei Yeah, it is both common and "wrong" (considered insecure) :) Problem is that it allows one to hijack output from the binary and redirect it somewhere else. If binary is running as privileged user, it can possibly be used as an attack vector. Understood, thanks. Not like this is real security concern in dmd case but guidelines like "don't make /tmp/ path predictable" exist exactly so that one can have simple safe default and not worry about possibilities. This may be a misunderstanding. I'm saying is to switch from unpredictable paths rooted in /tmp/ to equally unpredictable paths rooted in /tmp/.dmd-test-run/. Thanks, Andrei
Re: Anyone has time for a unittesting issue?
On Saturday, 1 October 2016 at 18:24:07 UTC, Andrei Alexandrescu wrote: Granted, no contest. Seems to me we could be a better denizen of said junkyard. What I noticed other apps do is create one directory in /tmp and then place their junk in there. -- Andrei Yeah, it is both common and "wrong" (considered insecure) :) Problem is that it allows one to hijack output from the binary and redirect it somewhere else. If binary is running as privileged user, it can possibly be used as an attack vector. Not like this is real security concern in dmd case but guidelines like "don't make /tmp/ path predictable" exist exactly so that one can have simple safe default and not worry about possibilities. Sure, it makes things less pretty, but beauty of /tmp/ layout is hardly an important goal to follow. It seems like more practical issue is simply that no regular destruction of /tmp/ happens on your system.
Re: Anyone has time for a unittesting issue?
On 10/01/2016 01:48 PM, Dicebot wrote: On Saturday, 1 October 2016 at 17:45:30 UTC, Dicebot wrote: On Saturday, 1 October 2016 at 17:35:29 UTC, Andrei Alexandrescu wrote: I think /tmp/ is mounted per user so we should be good. Anyhow... care to send a PR upstream? -- Andrei You can't make any assumptions about how /tmp/ is mounted, it is completely up to distro/administrator. As for original post - it is not an issue. The whole /tmp/ content can be wiped between sessions (it is not uncommon to mount it into RAM) - there should never be need to only remove dmd related parts. The whole point of /tmp/ is to be a random junkyard. Granted, no contest. Seems to me we could be a better denizen of said junkyard. What I noticed other apps do is create one directory in /tmp and then place their junk in there. -- Andrei
Re: Anyone has time for a unittesting issue?
On Saturday, 1 October 2016 at 17:45:30 UTC, Dicebot wrote: On Saturday, 1 October 2016 at 17:35:29 UTC, Andrei Alexandrescu wrote: I think /tmp/ is mounted per user so we should be good. Anyhow... care to send a PR upstream? -- Andrei You can't make any assumptions about how /tmp/ is mounted, it is completely up to distro/administrator. As for original post - it is not an issue. The whole /tmp/ content can be wiped between sessions (it is not uncommon to mount it into RAM) - there should never be need to only remove dmd related parts. The whole point of /tmp/ is to be a random junkyard.
Re: Anyone has time for a unittesting issue?
On Saturday, 1 October 2016 at 17:35:29 UTC, Andrei Alexandrescu wrote: I think /tmp/ is mounted per user so we should be good. Anyhow... care to send a PR upstream? -- Andrei You can't make any assumptions about how /tmp/ is mounted, it is completely up to distro/administrator.
Re: Anyone has time for a unittesting issue?
On 10/01/2016 12:58 PM, Guillaume Boucher wrote: On Saturday, 1 October 2016 at 16:46:56 UTC, Guillaume Boucher wrote: On Saturday, 1 October 2016 at 16:43:29 UTC, Stefan Koch wrote: On Saturday, 1 October 2016 at 16:05:25 UTC, Andrei Alexandrescu wrote: https://issues.dlang.org/show_bug.cgi?id=16568 TIA! -- Andrei Please post some code/instructions to reproduce the issue. The fix for this will be quite simple create a directory with the DDMMYY timestamp as directory name, and bundle the files under it. Any predictable name is a security issue. Use e.g. mkdtemp. Well I guess checking for 700 is sufficient. But this does not scale with multiple users. I think /tmp/ is mounted per user so we should be good. Anyhow... care to send a PR upstream? -- Andrei
Re: Anyone has time for a unittesting issue?
On Saturday, 1 October 2016 at 16:46:56 UTC, Guillaume Boucher wrote: On Saturday, 1 October 2016 at 16:43:29 UTC, Stefan Koch wrote: On Saturday, 1 October 2016 at 16:05:25 UTC, Andrei Alexandrescu wrote: https://issues.dlang.org/show_bug.cgi?id=16568 TIA! -- Andrei Please post some code/instructions to reproduce the issue. The fix for this will be quite simple create a directory with the DDMMYY timestamp as directory name, and bundle the files under it. Any predictable name is a security issue. Use e.g. mkdtemp. Well I guess checking for 700 is sufficient. But this does not scale with multiple users.
Re: Anyone has time for a unittesting issue?
On Saturday, 1 October 2016 at 16:43:29 UTC, Stefan Koch wrote: On Saturday, 1 October 2016 at 16:05:25 UTC, Andrei Alexandrescu wrote: https://issues.dlang.org/show_bug.cgi?id=16568 TIA! -- Andrei Please post some code/instructions to reproduce the issue. The fix for this will be quite simple create a directory with the DDMMYY timestamp as directory name, and bundle the files under it. Any predictable name is a security issue. Use e.g. mkdtemp.
Re: Anyone has time for a unittesting issue?
On Saturday, 1 October 2016 at 16:05:25 UTC, Andrei Alexandrescu wrote: https://issues.dlang.org/show_bug.cgi?id=16568 TIA! -- Andrei Please post some code/instructions to reproduce the issue. The fix for this will be quite simple create a directory with the DDMMYY timestamp as directory name, and bundle the files under it.
Anyone has time for a unittesting issue?
https://issues.dlang.org/show_bug.cgi?id=16568 TIA! -- Andrei
