< luke-jr>
did anyone ever confirm that #17740 doesn't reintroduce CVE-2012-1910?
< gribble>
https://github.com/bitcoin/bitcoin/issues/17740 | build: remove configure checks for win libraries we dont link against by fanquake · Pull Request #17740 · bitcoin/bitcoin · GitHub
< fanquake>
luke-jr: Those checks don't determine what is linked against
< fanquake>
From what I remember we did discuss that in the issue, and the only libs that were removed were unused.
< luke-jr>
fanquake: it's supposed to
< fanquake>
You're right. I've just re-read, and at the time I did check that none of those libs were being passed to the linker
< luke-jr>
which makes my question: have we reintroduced it previously? XD
< fanquake>
luke-jr: I'll look. btw if you've got time can you rebase #15704
< gribble>
https://github.com/bitcoin/bitcoin/issues/15704 | Move Win32 defines to configure.ac to ensure they are globally defined by luke-jr · Pull Request #15704 · bitcoin/bitcoin · GitHub
< luke-jr>
so -lmingwthrd is definitely being passed here
< fanquake>
luke-jr: looking at the mingw-w64 source, in regards to mingwthrd I'm seeing: As _CRT_MT is getting defined in libgcc when using shared version, or it is getting defined by startup code itself, this library is a dummy version for supporting the link library for gcc's option -mthreads. As we support TLS-cleanup even without specifying this library, this library is deprecated and just kept for compatibility.
< fanquake>
The only code in mingwthrd_mt.c is: int _CRT_MT_OLD = 1;
< fanquake>
mingwthrd_mt.c is the source for libmingwthrd.a
< fanquake>
I just checked that binaries produced with and without -lmingwthrd are basically identical. I see 5 bytes difference, and that's the git commit and what looks like 2 timestamps.
< stevenroose>
It seems that at least part of the other commits are intended to not require users to remove the fee_estimatoin.dat file, is that correct?
< aj>
stevenroose: seems kinda rude to have to spend a week before you can get fee estimates again?
< aj>
stevenroose: (if you just did that, users who upgrade would continue to not get estimates for below 1s/B until they deleted their fee_estimates.dat, they'd continue using the same buckets that were current when f_e.dat was generated)
< stevenroose>
aj: where does that week come from?
< stevenroose>
does that mean a week before getting any estimate? Or a week before getting reliable ones?
< aj>
stevenroose: i'm not sure of the numbers, but there's a weekly cycle so i wouldn't really trust the numbers much without that much data
< bitcoin-git>
[bitcoin] brakmic opened pull request #18429: build: remove double LIBBITCOIN_SERVER from bench-Makefile (master...bench-makefile) https://github.com/bitcoin/bitcoin/pull/18429
< bitcoin-git>
bitcoin/master 244e88e Wladimir J. van der Laan: Merge #18402: gui: display mapped AS in peers info window
< bitcoin-git>
[bitcoin] laanwj merged pull request #18402: gui: display mapped AS in peers info window (master...gui-peers-display-mapped_as) https://github.com/bitcoin/bitcoin/pull/18402
< wumpus>
i think it would be nice to have #15600 in 0.20.0, as it prevents leaking private key data into core dumps; does anyone here know enough about linux internals and madvise() to review / test it?
< gribble>
https://github.com/bitcoin/bitcoin/issues/15600 | lockedpool: When possible, use madvise to avoid including sensitive information in core dumps by luke-jr · Pull Request #15600 · bitcoin/bitcoin · GitHub
< vasild>
wumpus: I was just wondering what to pick up next :)
< * luke-jr>
gives up on Windows Update nonsense. How do people even use Windows? smh
< vasild>
luke-jr: ^ maybe sneak a removal of "if (addr)" into #15600, or maybe not. It is definitely out of scope.
< gribble>
https://github.com/bitcoin/bitcoin/issues/15600 | lockedpool: When possible, use madvise to avoid including sensitive information in core dumps by luke-jr · Pull Request #15600 · bitcoin/bitcoin · GitHub
< phantomcircuit>
luke-jr, windows update is akamai who seem to be randomly on fire from huge demand at the moment
< fjahr>
wumpus: that looks like the same issue that I saw here https://github.com/bitcoin-core/bitcoin-maintainer-tools/pull/54 I think while there could be something wrong on travis side, Github should definitely not hide the check either way. I will report it to Github as a bug.
< luke-jr>
phantomcircuit: the download works fine, it just errors installing
< wumpus>
vasild: huh, good catch, "On error, the value MAP_FAILED (that is, (void *) -1)". Seems out of scope for this issue but might still make sense to open a PR for it.
< vasild>
yes, I was thinking the same
< wumpus>
fjahr: ah yes it might be a github issue too, that they skip calling travis for some reason. i suspected a travis issue because last time travis was 'under maintenance' the same happened.
< hebasto>
wumpus: since 18331 is merged, mind looking into ##18349 ?
< bitcoin-git>
bitcoin/master fae1e99 MarcoFalke: ci: Only clone bitcoin-core/qa-assets when fuzzing
< bitcoin-git>
bitcoin/master 7c94225 MarcoFalke: Merge #18430: ci: Only clone bitcoin-core/qa-assets when fuzzing
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #18430: ci: Only clone bitcoin-core/qa-assets when fuzzing (master...2003-ciFuzzClone) https://github.com/bitcoin/bitcoin/pull/18430
< fjahr>
wumpus: yes, but even if there is an issue on travis' side then at least Github should warn that one of the checks didn't run in my opinion. I had to send them an email. Seems like they can't think of a better way to track issues %)
< bitcoin-git>
bitcoin/master d056df0 Ben Woosley: Replace std::to_string with locale-independent alternative
< bitcoin-git>
bitcoin/master 2e97d80 Wladimir J. van der Laan: Merge #18134: Replace std::to_string with locale-independent alternative
< bitcoin-git>
[bitcoin] laanwj merged pull request #18134: Replace std::to_string with locale-independent alternative (master...2020-02-to-string) https://github.com/bitcoin/bitcoin/pull/18134
< vasild>
luke-jr: jonatack: wumpus: Do you think that #15600 may create too crippled core files and thus impede debugging? In gdb, inspecting a core file and trying to read the value of a pointer that points to a removed page I get "Cannot access memory at address 0x803200000" which looks like a bogus/dangling pointer.
< gribble>
https://github.com/bitcoin/bitcoin/issues/15600 | lockedpool: When possible, use madvise to avoid including sensitive information in core dumps by luke-jr · Pull Request #15600 · bitcoin/bitcoin · GitHub
< vasild>
I do not want to bloat the PR with this, but maybe surround the madvise(DONTDUMP) call with "#ifdef DEBUG"?
< vasild>
I mean #ifndef
< jonatack>
vasild: interesting review, good catch! am trying to reproduce your tests with the steps you gave
< vasild>
I am ok with the patch as it is. Just got this thought which you may want to consider.
< vasild>
jonatack: abort() is king! :)
< jonatack>
:)
< sipa>
vasild: due to our secure allocator, the only thing actually stored in the protected pages is private keys
< sipa>
so i think the degree to which it would hurt debugging is probably very limited?
< vasild>
sipa: this is why I only mention it here instead of "bloating the PR". Btw, at least some random bytes are also allocated with the secure_allocator, see https://bpaste.net/RWOQ
< luke-jr>
vasild: that's what it's supposed to do..
< sipa>
right, the RNG state, of course
< sipa>
my point is mainly that it's just *data* that's stored there
< sipa>
not for example allocation tables
< sipa>
or complex data structures
< luke-jr>
I don't know any way to execute anything in a coredump-loaded gdb, but if there is, you could also fix it by mmaping something
< luke-jr>
(IIRC, mmap lets you choose a location)
< sipa>
luke-jr: i don't follow
< luke-jr>
sipa: if you're worried that you may get a SEGV running some funcitons to debug with, you can simply first mmap a blank file to the DONTDUMP pages
< vasild>
ok, so leave the PR as is
< sipa>
luke-jr: i don't think you can execute anything in a core-dump inspection; just observe
< sipa>
and non-dumped data you cannot observe
< luke-jr>
sipa: right, I don't know a way how either; but in that scenario, you can't get a SEGV either
< luke-jr>
being unable to observe non-dumped data, is the point of the PR
< sipa>
it may be undesirable in debug settings, which is certainly true
< wumpus>
vasild: I think that's an acceptable risk personally, we have to err on the side of security/privacy in general
< wumpus>
I think people rarely use core files for debugging, but if they do, if that means gdb gives a few more warnings I think that's acceptable
< wumpus>
yes, as sipa already says, it only applies to private key data and some very limited things
< vasild>
+1
< jonatack>
reproduced vasild's findings
< jonatack>
describing with a comment in the PR
< vasild>
So it works! \o/
< jonatack>
yes! good stuff
< fjahr>
Is there a github access token available in travis and if yes, what is the variable name? I would need it to prevent running into rate limiting with #18399
< bitcoin-git>
[bitcoin] practicalswift opened pull request #18432: util: Make current implicit C++ locale assumption in tfm::format(...) explicit (master...cpp-locale) https://github.com/bitcoin/bitcoin/pull/18432