Hey Willy, On 12/7/15, 2:22 PM, "Willy Tarreau" <[email protected]> wrote: >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 completely understand where you¹re coming from so lets try to fix this as best as we can. So I followed this to the gcc bugzilla as you suggested and voila: bogus buffer overflow warning and abort on flexible array member in a child object https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60792 Which is exactly what I¹m doing. This isn¹t a bug though, just a different use pattern than what GCC allows for. In this case, the suggestion was to use memcpy. I feel that my patch does exactly what is suggested in that thread. > >(...) > >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. This pattern I copied from other code in HAProxy, so I thought it would be acceptable. Also, I¹m not sure what you mean by "don't find a 0 in the trash". It¹s looking through str for the null terminator, which came from reading the CN/SAN. > >(...) > >@@ -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. Given the above, did you still want me to remove the code to add the trailing 0? -Dave
