Re: [PATCH] Upstreaming a large amount of patches

Hi Felix,

On 03/29/2016 06:06 PM, Felix von Leitner wrote:
> I just imported a ton of your patches.

Thanks a lot, this reduces the amount of patches in Debian by quite
a bit! I'll probably have a couple more coming soon, but this was
the largest set.

>> ========================== UNCLEAR STUFF ==============================
> 
>>  * Out of bounds memory access with fast string functions without
>>    WANT_SMALL_STRING_ROUTINES
> 
> Generally speaking, these are all harmless.
> As long as the first element of the word is not out of bounds, and the
> accesses are aligned, you are good. Access limitations happen on page
> boundaries, and you can't cross a page boundary like that.
> 
> So it is safe to do this.
> 
> However, valgrind will whine. That's why I added a #define
> WANT_VALGRIND_SUPPORT, which will detect, at run time, whether we are
> running under valgrind, and then switch back to slow and unverbose
> versions.
> 
> This has not been implemented for all variants, but since for
> development I'm only using x86_64, and it's implemented there, and I
> can't imagine anybody running valgrind on ARM (it's so slow already!),
> there has never been much pressure to support this on the other
> routines.
> 
> Patches are welcome!

So I've read both your reply and Lorenzo's reply on this issue, as
well as looked at how the OpenBSD allocator works (which uses mmap
to do out of bounds checks), and I played around with some trivial
mmap code on my own. I believe you are correct in what you're
saying, and while I'm not confident enough in my assessment to
write code myself that uses the same trick, I am going to defer to
your judgment here.

I'll work on a patch that also includes the valgrind check for the
generic versions for this, and afterwards drop the original patch
from Debian, because I really want to be as close to upstream as
possible and the patch in Debian to remove the code is far too
large for my taste.

>>       Ubuntu's gcc passes -fstack-protector by default, so if dietlibc
>>       is compiled without WANT_SSP, it should explicitly pass
>>       -fno-stack-protector to the compiler.
> 
> No it should not.
> It should for the one start file that initializes the stack cookie.
> This is already done, with a facepalm comment for good measure :)

No, you misunderstand me: if you compile the whole project without
WANT_SSP, then the stack cookie initialization code is not included,
so if _other_ code is then compiled with -fstack-protector, since
the cookie isn't initialized, the code will fail.

For WANT_SSP = yes what you're doing is completely correct, the
patch in Debian is supposed to catch the case of WANT_SSP = no.

In Debian up to current unstable version of dietlibc (which is four
years old), for reasons related to older toolchains (which were
probably not relevant anymore even four years ago) WANT_SSP was
disabled in dietfeatures.h a long time ago in Debian.

In the experimental upload I did a while back, I reenabled WANT_SSP,
because I consider stack smashing protection to be a good thing and
current toolchains don't have a problem with it. So I will
definitely drop the patch from Debian, because I've no interest in
supporting WANT_SSP = no in the official Debian packages.

However, if a user wants to compile dietlibc themselves on Ubuntu,
with WANT_SSP = yes it will work currently, but it won't with
WANT_SSP = no, because of the default -fstack-protector. As I said
in my initial mail, I don't think you should carry this patch
upstream necessarily, but it might be a good idea to document this
in a comment in dietfeatures.h to tell users they need to manually
pass -fno-stack-protector on Ubuntu and derivatives thereof if they
disable it. Something like:

--- dietfeatures.h.orig 2016-03-29 19:05:02.892611289 +0200
+++ dietfeatures.h      2016-03-29 19:06:28.835441347 +0200
@@ -143,7 +143,9 @@
  * -fstack-protector. */
 /* If you compile dietlibc without WANT_SSP and then try to link code
  * compiled with -fstack-protector against it, the binary will segfault
- * when calling that code. */
+ * when calling that code. Be careful that -fstack-protector is a
+ * default option on Ubuntu, so disabling this there will require you
+ * to explicitly pass -fno-stack-protector as a compiler option. */
 #if (__GNUC__>4) || ((__GNUC__==4) && (__GNUC_MINOR__>=1))
 #define WANT_SSP
 #endif


Side note: if WANT_SSP is enabled, is there a reason why dietlibc
itself (except for the setup code) isn't compiled with SSP, i.e.
why -fstack-protector isn't explicitly passed to gcc when WANT_SSP
is enabled? To me it seems to be kind of suboptimal if only programs
using dietlibc make use of SSP, but dietlibc itself doesn't...?

(I can provide a patch if you're agreeable.)

>> https://gist.githubusercontent.com/chris-se/e35381c75583a4c4bce2/raw/8bf327a048f396220c48fb4ffbd504d12b042007/unclear-A02-native-x32.patch
> I don't think it makes sense to have this kind of thing in the mainline
> dietlibc, but feel free to keep on doing this for your x32 distro.
> 
> If your distro is x32 by default, I assume that means you pass -mx32 in
> gcc by default, too? Shouldn't dietlibc compile and work just fine out
> of the box then? Wouldn't it be just a cosmetic issue what the directory
> is called?

It's not just a cosmetic issue, because it still tries to compile
stuff from the x86_64 directory with the default compiler options,
which doesn't work if the compiler defaults to -mx32. And then you
have the problem that the build system considers x32 to be a purely
cross architecture, so 'make x32' tries to run bin-x86_64/diet due
to the fact that the kernel is x86_64.

If I add that patch and run make MYARCH=x32 on an x32 system, it
will build bin-x32/diet and also use that for the build itself.
The changes in diet.c is also needed so diet knows what start code
it needs to add by default.

OTOH, the patch is not invasive and quite trivial, so to be honest
I don't really care that much whether it's upstream or not (I did
put it in the "unclear" category for a reason) and am fine with
carrying it downstream. Just note that without the patch it's not
possible to build dietlibc trivially by a compiler that uses -mx32
by default.

Regards,
Christian