Project

General

Profile

Bug #3860

ztest_vdev_aux_add_remove should use MAXPATHLEN instead of sizeof in snprintf()

Added by Richard Yao over 7 years ago. Updated over 7 years ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Category:
zfs - Zettabyte File System
Start date:
2013-07-02
Due date:
% Done:

0%

Estimated time:
Difficulty:
Bite-size
Tags:
needs-triage
Gerrit CR:

Description

GCC 4.8.0's new diagnostics detected a programming error in ztest.c when building ZFSOnLinux with on Gentoo Linux. I have attached a patch to fix it. Here are links to the downstream issue and the corresponding pull request:

https://github.com/zfsonlinux/zfs/issues/1480
https://github.com/zfsonlinux/zfs/pull/1561

On an unrelated note, I will probably refrain from filing issues for larger patches until I am comfortable enough with Illumos development to test them before filing issues.


Files

patch (1.23 KB) patch One-line patch to fix the problem Richard Yao, 2013-07-02 04:34 AM
#1

Updated by Richard Yao over 7 years ago

The title has a typo. It should be snprintf(), not sprintf().

#2

Updated by Marcel Telka over 7 years ago

  • Subject changed from ztest_vdev_aux_add_remove should use MAXPATHLEN instead of sizeof in sprintf() to ztest_vdev_aux_add_remove should use MAXPATHLEN instead of sizeof in snprintf()
#3

Updated by Yuri Pankov over 7 years ago

There's nothing wrong with using sizeof() that way here.

#4

Updated by Igor Pashev over 7 years ago

Yuri Pankov wrote:

There's nothing wrong with using sizeof() that way here.

Unless path is a pointer:

https://github.com/ryao/zfs/blob/c236c99e097f596e85fd1b5de398ce908104a119/cmd/ztest/ztest.c#L2554

#5

Updated by Richard Yao over 7 years ago

Yuri Pankov wrote:

There's nothing wrong with using sizeof() that way here.

Using sizeof on an dynamic allocation is wrong. Here is a small C program that you can compile:

#include <stdlib.h>
#include <stdio.h>

int main (){
char *buffer = malloc(10240);
printf("sizeof buffer = %d\\n", sizeof buffer);
return 0;
}

The output on my system is "sizeof buffer = 8", which is the size of the pointer, not the size of the allocation. We should not be passing the size of the pointer to snprintf().

#6

Updated by Richard Yao over 7 years ago

Igor, thanks for spotting that. Yuri was right. I neglected to notice that ZFSOnLinux had changed this to be a dynamic allocation from Illumos' static allocation when I checked if this patch applied to Illumos. The use of a stack allocation makes the original code fine. It was late at night when I filed this (and early in the morning now), so I missed that. Would someone close this issue? It is invalid.

#7

Updated by Christopher Siden over 7 years ago

  • Status changed from New to Closed

Also available in: Atom PDF