Hi Dave,
On Mon, Dec 07, 2015 at 06:49:54PM +0000, Dave Zhu (yanbzhu) wrote:
> Bryan found an interesting bug in the code, which I??ve root caused to an
> optimization bug(?)/eccentricity in gcc 4.8.4.
I'm sorry but this is not acceptable. We're hiding a deeper bug somewhere.
Strncpy() is used at other places in the code (a few I agree) and is used
by *a lot* of programs, it's a basic building block. If it's broken in one
version of gcc, it will quickly be fixed, and/or we may have to deploy
workarounds for other places as well. Instead I guess that strncpy() is
detecting a bug that memcpy() doesn't detect so until we find the real
root cause of all of this with a proof that strncpy() contains the real
bug and why it doesn't affect other places, we must not hide it because
I'm pretty sure it will strike back later. Or at least I'd like to see
a pointer to the corresponding bugzilla entry in gcc's bugtracker. I
could imagine one reason being that the sni_keytype would be packed
(for what reason ?) and that the string pointer would start unaligned
and would cause some of strncpy()'s vector optims to fail. But that
sounds extremely strange. By experience when a program dies from such
a small change, it's a latent memory dereference that gets optimized
differently and which manifests itself via memory manipulation functions.
Note that I'm not defending gcc, you might have seen a number of angry
comments from me against it in the code. I just want to be sure we're
addressing the real issue instead of hiding it.
(...)
I'm seeing a buffer overflow in the code prior to the bug seeing the crash,
though its unlikely to trigger but it's here :
for (i = 0; i < trash.size; i++) {
if (!str[i])
break;
trash.str[i] = tolower(str[i]);
}
trash.str[i] = 0;
As you can see, if you don't find a 0 in the trash, it will overwrite the
first character not part of the trash buffer.
(...)
@@ -1808,7 +1808,10 @@ static void ssl_sock_populate_sni_keytypes_hplr(const
char *str, struct eb_root
if (!node) {
/* CN not found in tree */
s_kt = malloc(sizeof(struct sni_keytype) + i + 1);
- strncpy((char *)s_kt->name.key, trash.str, i);
+ /* Using memcpy here instead of strncpy.
+ * strncpy will cause sig_abrt errors under certain version of
gcc with -O2
+ */
+ memcpy(s_kt->name.key, trash.str, i);
s_kt->name.key[i] = 0;
s_kt->keytypes = 0;
ebst_insert(sni_keytypes, &s_kt->name);
It might be useful to display s_kt, s_kt->name.key, sizeof(struct sni_keytype)
and i at this place to understand what upsets gcc/strncpy(). If we have to
switch to memcpy() (which I prefer in general), you can remove the manual
addition of the trailing 0 by adding 1 to the size of the memcpy() since
it's guaranteed to cover the zero.
Thanks,
Willy
