On Tue, 25 Jan 2022 09:48:43 +0200
Tzvetomir Stoyanov <tz.stoyanov@xxxxxxxxx> wrote:
> > > >
> > > > > + free(bytes);
> > > > > + handle->pointer = 0;
> > > > > + handle->capacity = s_uncompressed;
> > > >
> > > > If size is the greater of the two (compressed could be larger) and
> > > > capacity is the allocated buffer, shouldn't we make it equal to size
> > > > and not uncompressed?
> > >
> > > The name "capacity" in reading logic is a bit confusing, as it
> > > actually is used to track the size of uncompressed data. The buffer
> > > may be larger, but if the uncompressed is smaller - the rest is
> > > garbage and must not be read.
> >
> > Then the extend function is broken:
>
> For this particular use case, the logic is not correct - the realloc()
> could be redundant, if the real buffer size is bigger than the
> capacity. But as usually a compression handle is used for reading or
> for writing only, this is unlikely to happen. At least I cannot
> imagine a use case where the same compression handle will be used for
> reading a compress block and for writing compressed data. But even if
> we hit that case - there is no data corruption or memory leak, just a
> redundant realloc(). To solve that, reading and writing capacities
> could be introduced ?
> struct tracecmd_compression {
> ...
> unsigned int capacity_read;
> unsigned int capacity_write;
> ...
> }
A redundant realloc is not an issue, in fact, they are pretty fast (it
detects that the requested size still fits what is allocated and just
returns).
>
> But that change will be for a very rare case - when the compressed
> data size is bigger than the uncompressed (usually happens only with a
> very small data) + the same compression handle is used for compression
> and decompression (not used in that way in trace-cmd).
>
> >
> > static inline int buffer_extend(struct tracecmd_compression *handle, unsigned int size)
> > {
> > int extend;
> > char *buf;
> >
> > if (size <= handle->capacity)
> > return 0;
> >
> > extend = ((size - handle->capacity) / BUFSIZ + 1) * BUFSIZ;
> > buf = realloc(handle->buffer, handle->capacity + extend);
> > if (!buf)
> > return -1;
> > handle->buffer = buf;
> > handle->capacity += extend;
> >
> > return 0;
> > }
> >
> > As it sets the capacity to the allocated size, not what is stored.
> >
> > Perhaps you meant:
> >
> > handle->capacity += size;
The above should still work, instead of doing the += extend.
The point I was making is that capacity should either be the requested size
allocated, or what is actually allocated. The above function sets it to
what is allocated, and the other code sets it to what was requested. We
should pick it to be one or the other.
-- Steve
> >
> > ?
> >
> > >
> > > >
> > > >
> > > > > + return 0;
> > > > > +error:
> > > > > + tracecmd_compress_reset(handle);
> > > > > + free(bytes);
> > > > > + return -1;
> > > > > +}
> > > > > +
|