| Message ID | 1474466758-26544-1-git-send-email-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 21/09/2016 14:22, Paul Eggert wrote: > On 09/21/2016 07:05 AM, Adhemerval Zanella wrote: >> + clen += len; > > That might cause problems , as the C standard says you can't access a flexible array member past the last byte that is properly aligned for the containing structure, and GCC relies on this when generating code (see GCC bug 66661). One way to work around the problem is to change the earlier: > > + size_t clen = sizeof (fmemopen_cookie_t); > > to: > > + size_t clen = sizeof (fmemopen_cookie_t) + _Alignof (fmemopen_cookie_t) - 1; > > This may overallocate a bit, but that's OK here. > Thanks for pointing this out, I think this is the safe approach regarding Florian comment #10 and #11. I will change it.
On 21/09/2016 11:57, Florian Weimer wrote: > On 09/21/2016 04:05 PM, Adhemerval Zanella wrote: >> This patch uses C99 variable-lenght array on internal fmemopen cookie >> allocation to avoid to use two mallocs for the case if an internal >> buffer must be allocated. No functional changes are done. >> >> Tested on x86_64 and i686. >> >> * libio/fmemopen.c (fmemopen_cookie_t): Remove mybuffer and add >> variable-length array member. >> (fmemopen_close): Do no free the internal buffer. >> (__fmemopen): Use C99 variable-length array to allocate both the >> fmemopen cookie and internal buffer (for null arguments). > > It's a flexible array member, not a variable-length array. Looks good to me. Right, I will correct naming in commit message. > > What about libio/oldfmemopen.c? Do you want to touch this as well? I won't bother with it, testing would be tricky and I do not see a compelling reason to optimize compat code.
On 21/09/2016 15:23, Florian Weimer wrote: > On 09/21/2016 08:17 PM, Adhemerval Zanella wrote: >> >> >> On 21/09/2016 14:22, Paul Eggert wrote: >>> On 09/21/2016 07:05 AM, Adhemerval Zanella wrote: >>>> + clen += len; >>> >>> That might cause problems , as the C standard says you can't access a flexible array member past the last byte that is properly aligned for the containing structure, and GCC relies on this when generating code (see GCC bug 66661). One way to work around the problem is to change the earlier: >>> >>> + size_t clen = sizeof (fmemopen_cookie_t); >>> >>> to: >>> >>> + size_t clen = sizeof (fmemopen_cookie_t) + _Alignof (fmemopen_cookie_t) - 1; >>> >>> This may overallocate a bit, but that's OK here. >>> >> >> Thanks for pointing this out, I think this is the safe approach regarding >> Florian comment #10 and #11. I will change it. > > Please don't, it's a defect in the standard (and GCC"s Address Sanitizer has a bug as well). I do not have a strong opinion here, my idea to use this workaround is to avoid possible issues with faulty compilers. However, if the consensus is to not push this forward, I can use my original patch.
On 21/09/2016 16:27, Florian Weimer wrote: > On 09/21/2016 08:57 PM, Paul Eggert wrote: >> On 09/21/2016 11:47 AM, Florian Weimer wrote: >>> We don't know the nature of the GCC issue, so we cannot work around it >>> reliably. The most likely explanation is that Address Sanitizer does >>> not account for a valid GCC optimization. >> >> That's not the sense that I get from looking at Bug 66661. GCC is >> assuming it can do an aligned word load of the trailing bytes of a >> flexible array member in a properly-aligned structure. > > But that assumption is completely correct! It's just that Address Sanitizer does not account for it (based on what I've seen so far). > >> Although the followups to Bug 66661 suggest that there may be >> further problems with overaligned structures, the code here is not using >> _Alignas so these further problems are not an issue here. > > _Alignas is used only to demonstrate the issue more clearly. > > glibc currently does not compile with Address Sanitizer anyway, so until that is fixed (or someone has a better explanation of the root cause of GCC PR66661), I would suggest to assume that this issue does not matter to glibc at all. I check with a different memory profiler (valgrind) and at least with gcc 5.4 fmemopen with this change shows no wrong memory access. It also shows that the issue might be indeed in ASAN. I think we can push this optimization as if and I will continue track BZ#66661 to check for further development if the patch would require more changes.
diff --git a/libio/fmemopen.c b/libio/fmemopen.c index 0f65590..d7df1be 100644 --- a/libio/fmemopen.c +++ b/libio/fmemopen.c @@ -35,11 +35,11 @@ typedef struct fmemopen_cookie_struct fmemopen_cookie_t; struct fmemopen_cookie_struct { char *buffer; /* memory buffer. */ - int mybuffer; /* allocated my buffer? */ int append; /* buffer open for append? */ size_t size; /* buffer length in bytes. */ _IO_off64_t pos; /* current position at the buffer. */ size_t maxpos; /* max position in buffer. */ + char data[]; }; @@ -136,11 +136,7 @@ fmemopen_seek (void *cookie, _IO_off64_t *p, int w) static int fmemopen_close (void *cookie) { - fmemopen_cookie_t *c = (fmemopen_cookie_t *) cookie; - - if (c->mybuffer) - free (c->buffer); - free (c); + free (cookie); return 0; } @@ -153,23 +149,24 @@ __fmemopen (void *buf, size_t len, const char *mode) fmemopen_cookie_t *c; FILE *result; - c = (fmemopen_cookie_t *) calloc (sizeof (fmemopen_cookie_t), 1); - if (c == NULL) - return NULL; - - c->mybuffer = (buf == NULL); - - if (c->mybuffer) + size_t clen = sizeof (fmemopen_cookie_t); + if (buf == NULL) { - c->buffer = (char *) malloc (len); - if (c->buffer == NULL) + if (__glibc_unlikely (len >= (SIZE_MAX - clen))) { - free (c); + __set_errno (ENOMEM); return NULL; } - c->buffer[0] = '\0'; + clen += len; } - else + + c = (fmemopen_cookie_t *) calloc (clen, 1); + if (c == NULL) + return NULL; + + c->buffer = c->data; + + if (buf != NULL) { if (__glibc_unlikely ((uintptr_t) len > -(uintptr_t) buf)) { @@ -214,12 +211,7 @@ __fmemopen (void *buf, size_t len, const char *mode) result = _IO_fopencookie (c, mode, iof); if (__glibc_unlikely (result == NULL)) - { - if (c->mybuffer) - free (c->buffer); - - free (c); - } + free (c); return result; }