On Tue, 2022-05-31 at 16:03 +1000, Dave Chinner wrote:
> On Fri, May 20, 2022 at 12:00:31PM -0700, Allison Henderson wrote:
> > This patch implements a new set of log printing functions to print
> > the
> > ATTRI and ATTRD items and vectors in the log. These will be used
> > during
> > log dump and log recover operations.
> >
> > Though most attributes are strings, the attribute operations accept
> > any binary payload, so we should not assume them printable. This
> > was
> > done intentionally in preparation for parent pointers. Until
> > parent
> > pointers get here, attributes have no discernible format. So the
> > print
> > routines are just a simple print or hex dump for now.
> >
> > Signed-off-by: Allison Henderson <allison.henderson@xxxxxxxxxx>
>
> Oh, this reminds me how much I dislike logprint, having multiple,
> very subtly different ways to print the same information in slightly
> different formats.
I did notice that, but I thought I should try and be as consistent as
possible at least for now.
>
> Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>
>
Thank you!
> But this is a bug I needed to fix...
>
> .....
> > + if (src_f->alfi_value_len > 0) {
> > + printf(_("\n"));
> > + (*i)++;
> > + head = (xlog_op_header_t *)*ptr;
> > + xlog_print_op_header(head, *i, ptr);
> > + error = xlog_print_trans_attri_value(ptr,
> > be32_to_cpu(head->oh_len),
> > + src_f->alfi_value_len);
> > + }
>
> So this passes the length of the region and the length of the
> value. They are not the same, the value can be split across multiple
> regions as the value is split across multiple log writes. so....
>
> > +int
> > +xlog_print_trans_attri_value(
> > + char **ptr,
> > + uint src_len,
> > + int value_len)
> > +{
> > + int len = max(value_len, MAX_ATTR_VAL_PRINT);
> > +
> > + printf(_("ATTRI: value len:%u\n"), value_len);
> > + print_or_dump(*ptr, len);
>
> This dumps the value length from a buffer of src_len, overruns the
> end of the buffer and Bad Things Happen. (i.e. logprint segv's)
>
> This should be:
>
> int len = min(value_len, src_len);
>
> So that the dump doesn't overrun the region buffer....
>
> I'll fix it directly in my stack, I think this is the only remaining
> failure I'm seeing with my current libxfs 5.19 sync branch....
Ah, alrighty then thanks for the catch, I will wait for your updates
then. Thx!
Allison
>
> Cheers,
>
> Dave.
