Re: [PATCH 2/2] xfs_repair: clear new memory after realloc

[Brian Foster <bfoster@redhat.com>]

On Fri, Jun 19, 2015 at 10:21:36AM +0100, Mike Grant wrote:
> longform_dir2_entry_check in phase6.c has a calloc'ed array of xfs_buf
> pointers (bplist).  It reallocs this array if there turns out to be more
> blocks than expected.  However, realloc doesn't zero the new memory like
> calloc.  In unusual circumstances*, things may then blow up later due to
> random data populating the new part of the array.
> 
> This patch zeros the new part of the array.
> 
> * (bit speculative) as dir_read_buf zeros the element it's looking at, I
> think this can only happen if the realloc adds several members and one
> of the first is corrupt.  In my case, the realloc went from 35 to 37
> members, meaning db must have been 36 without being 35.  A read error
> then caused it to goto out_fix. The crash then occurred in the
> libxfs_putbuf when looping through the bplist structure, checking it for
> NULL pointers (and presumably tripping over the non-zeroed data at
> position 35?)
> 
> Signed-off-by: Mike Grant <mggr@pml.ac.uk>
> ---
>  repair/phase6.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/repair/phase6.c b/repair/phase6.c
> index 0d66f9c..4dd7551 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -2326,6 +2326,7 @@ longform_dir2_entry_check(xfs_mount_t	*mp,
>  		db = xfs_dir2_da_to_db(mp, da_bno);
>  		if (db >= num_bps) {
>  			/* more data blocks than expected */
> +			int num_bps_prev = num_bps;
>  			num_bps = db + 1;
>  			bplist = realloc(bplist, num_bps *
>  				sizeof(struct xfs_buf*));
> @@ -2334,6 +2335,10 @@ longform_dir2_entry_check(xfs_mount_t	*mp,
>  					_("realloc failed in %s (%zu bytes)\n"),
>  					__func__,
>  					num_bps * sizeof(struct xfs_buf*));
> +			/* clear new memory as previous bplist was calloc'ed */
> +			memset((void *) bplist + num_bps_prev
> +				* sizeof(struct xfs_buf*), 0, (num_bps -
> +				num_bps_prev) * sizeof(struct xfs_buf*));

Similar issue here with how the lines are split. In looking at it, I'm
not even sure how I would split this one up cleanly tbh.. ;) Maybe we
should just create another local variable for the bp size and clean up
the whole hunk. E.g.:

			int bpsz = sizeof(struct xfs_buf *);

			...

			memset((void *)bplist + num_bps_prev * bpsz, 0,
				(num_bps - num_bps_prev) * bpsz);

In fact, we could include bpsz in the first patch.

Brian

>  		}
>  
>  		if (isblock)
> -- 
> 2.1.0
> 
> (apologies for the following junk forcibly appended by my company)
> 
> 
> Please visit our new website at www.pml.ac.uk and follow us on Twitter  @PlymouthMarine
> 
> Winner of the Environment & Conservation category, the Charity Awards 2014.
> 
> Plymouth Marine Laboratory (PML) is a company limited by guarantee registered in England & Wales, company number 4178503. Registered Charity No. 1091222. Registered Office: Prospect Place, The Hoe, Plymouth  PL1 3DH, UK. 
> 
> This message is private and confidential. If you have received this message in error, please notify the sender and remove it from your system. You are reminded that e-mail communications are not secure and may contain viruses; PML accepts no liability for any loss or damage which may be caused by viruses.
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs