Re: [PATCH v2 18/18] xfs_logprint: Add log item printing for ATTRI and ATTRD

From: Alli <allison.henderson@oracle.com>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH v2 18/18] xfs_logprint: Add log item printing for ATTRI and ATTRD
Date: Tue, 31 May 2022 18:39:10 -0700	[thread overview]
Message-ID: <764c5e859d2875c1799c0fcc852f85ac705bfeee.camel@oracle.com> (raw)
In-Reply-To: <20220531060321.GB1098723@dread.disaster.area>

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@oracle.com>
> 
> 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@redhat.com>
> 
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.