xfs: avoid null *src in memcpy call in xlog_write
PATCH 1/1 xfs: avoid null *src in memcpy call in xlog_write
The gcc undefined behavior sanitizer caught this; surely any sane memcpy implementation will no-op if size == 0, but behavior with a *src of NULL is technically undefined (declared nonnull), so avoid it here.
We are actually in this situation frequently via xlog_commit_record(), because:
struct xfs_log_iovec reg = {
.i_addr = NULL,
.i_len = 0,
.i_type = XLOG_REG_TYPE_COMMIT,
};---
fs/xfs/xfs_log.c +4 -1
@@ -2424,7 +2424,10 @@ xlog_write(/* copy region */ ASSERT (copy_len >=0 ); -memcpy (ptr, reg->i_addr + copy_off, copy_len); +ASSERT (reg->i_addr + copy_off >0 || copy_len ==0 ); +/* size == 0 is ok, but *src == NULL is undefined */ +if (reg->i_addr + copy_off) +memcpy (ptr, reg->i_addr + copy_off, copy_len);xlog_write_adv_cnt (&ptr, &len, &log_offset, copy_len); copy_len += start_rec_copy +sizeof (xlog_op_header_t);
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Looks good to me.
Quoted 34 lines · Eric Sandeen
The gcc undefined behavior sanitizer caught this; surely any sane memcpy implementation will no-op if size == 0, but behavior with a *src of NULL is technically undefined (declared nonnull), so avoid it here.
We are actually in this situation frequently via
xlog_commit_record(), because:struct xfs_log_iovec reg = { .i_addr = NULL, .i_len = 0, .i_type = XLOG_REG_TYPE_COMMIT, };Signed-off-by Eric Sandeen <[email protected]>---
fs/xfs/xfs_log.c +4
-1@@ -2424,7 +2424,10 @@ xlog_write(/* copy region */ ASSERT (copy_len >=0 ); -memcpy (ptr, reg->i_addr + copy_off, copy_len); +ASSERT (reg->i_addr + copy_off >0 || copy_len ==0 ); +/* size == 0 is ok, but *src == NULL is undefined */ +if (reg->i_addr + copy_off) +memcpy (ptr, reg->i_addr + copy_off, copy_len);xlog_write_adv_cnt (&ptr, &len, &log_offset, copy_len); copy_len += start_rec_copy +sizeof (xlog_op_header_t);
Quoted 4 lines
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 22 lines · Eric Sandeen
The gcc undefined behavior sanitizer caught this; surely any sane memcpy implementation will no-op if size == 0, but behavior with a *src of NULL is technically undefined (declared nonnull), so avoid it here.
We are actually in this situation frequently via
xlog_commit_record(), because:struct xfs_log_iovec reg = { .i_addr = NULL, .i_len = 0, .i_type = XLOG_REG_TYPE_COMMIT, };Signed-off-by Eric Sandeen <[email protected]>---
fs/xfs/xfs_log.c +0
-0
fs/xfs/xfs_log.c xlog_write( · line 2431
Quoted 9 lines
fs/xfs/xfs_log.c +4
-1@@ -2424,7 +2424,10 @@ xlog_write(/* copy region */ ASSERT (copy_len >=0 ); -memcpy (ptr, reg->i_addr + copy_off, copy_len); +ASSERT (reg->i_addr + copy_off >0 || copy_len ==0 ); +/* size == 0 is ok, but *src == NULL is undefined */ +if (reg->i_addr + copy_off) +memcpy (ptr, reg->i_addr + copy_off, copy_len);xlog_write_adv_cnt (&ptr, &len, &log_offset, copy_len);
The comment doesn't explain anything about why copylen might be zero or reg->i_addr might be null. If copylen is zero, we should really skip the copy, not rely on some magic pointer arithmetic to tell us it's ok to copy...
/*
* Copy region.
*
* Unmount records just log an opheader, so can have
* empty payloads with no data region to copy. Hence we only
* copy the payload if the vector says it has data to copy.
*/
ASSERT(copy_len >= 0);
if (copy_len > 0) {
memcpy(ptr, reg->i_addr + copy_off, copy_len);
xlog_write_adv_cnt(&ptr, &len, &log_offset, copy_len);
}Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 38 lines · Dave Chinner
Quoted 22 lines · Eric Sandeen
The gcc undefined behavior sanitizer caught this; surely any sane memcpy implementation will no-op if size == 0, but behavior with a *src of NULL is technically undefined (declared nonnull), so avoid it here.
We are actually in this situation frequently via
xlog_commit_record(), because:struct xfs_log_iovec reg = { .i_addr = NULL, .i_len = 0, .i_type = XLOG_REG_TYPE_COMMIT, };Signed-off-by Eric Sandeen <[email protected]>---
fs/xfs/xfs_log.c +0
-0fs/xfs/xfs_log.c xlog_write( · line 2431
Quoted 9 lines
fs/xfs/xfs_log.c +4
-1@@ -2424,7 +2424,10 @@ xlog_write(/* copy region */ ASSERT (copy_len >=0 ); -memcpy (ptr, reg->i_addr + copy_off, copy_len); +ASSERT (reg->i_addr + copy_off >0 || copy_len ==0 ); +/* size == 0 is ok, but *src == NULL is undefined */ +if (reg->i_addr + copy_off) +memcpy (ptr, reg->i_addr + copy_off, copy_len);xlog_write_adv_cnt (&ptr, &len, &log_offset, copy_len);The comment doesn't explain anything about why copylen might be zero or
reg->i_addrmight be null. If copylen is zero, we should really skip the copy, not rely on some magic pointer arithmetic to tell us it's ok to copy...
Quoted 12 lines
/*
* Copy region.
*
* Unmount records just log an opheader, so can have
* empty payloads with no data region to copy. Hence we only
* copy the payload if the vector says it has data to copy.
*/
ASSERT(copy_len >= 0);
if (copy_len > 0) {
memcpy(ptr, reg->i_addr + copy_off, copy_len);
xlog_write_adv_cnt(&ptr, &len, &log_offset, copy_len);
}Yeah, I thought about that, why didn't I do it that way? Maybe I was thinking about *src still being NULL, but I guess in that case we'd find out that there's a problem very quickly.
You may as well just commit that version w/ your S-O-B and my Reported-by.
-Eric
Quoted 4 lines
Cheers,
Dave.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs