* [patch] [SCSI] libosd: check for kzalloc() failure
@ 2013-01-30 7:06 Dan Carpenter
2013-01-30 8:15 ` walter harms
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Dan Carpenter @ 2013-01-30 7:06 UTC (permalink / raw)
To: Boaz Harrosh
Cc: Benny Halevy, James E.J. Bottomley, osd-dev, linux-scsi,
kernel-janitors
There wasn't any error handling for this kzalloc().
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
index c06b8e5..d8293f2 100644
--- a/drivers/scsi/osd/osd_initiator.c
+++ b/drivers/scsi/osd/osd_initiator.c
@@ -144,6 +144,10 @@ static int _osd_get_print_system_info(struct osd_dev *od,
odi->osdname_len = get_attrs[a].len;
/* Avoid NULL for memcmp optimization 0-length is good enough */
odi->osdname = kzalloc(odi->osdname_len + 1, GFP_KERNEL);
+ if (!odi->osdname) {
+ ret = -ENOMEM;
+ goto out;
+ }
if (odi->osdname_len)
memcpy(odi->osdname, get_attrs[a].val_ptr, odi->osdname_len);
OSD_INFO("OSD_NAME [%s]\n", odi->osdname);
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [patch] [SCSI] libosd: check for kzalloc() failure
2013-01-30 7:06 [patch] [SCSI] libosd: check for kzalloc() failure Dan Carpenter
@ 2013-01-30 8:15 ` walter harms
2013-01-30 8:27 ` Dan Carpenter
2013-01-30 10:05 ` Benny Halevy
2013-01-30 15:56 ` Boaz Harrosh
2 siblings, 1 reply; 11+ messages in thread
From: walter harms @ 2013-01-30 8:15 UTC (permalink / raw)
To: Dan Carpenter
Cc: Boaz Harrosh, Benny Halevy, James E.J. Bottomley, osd-dev,
linux-scsi, kernel-janitors
Am 30.01.2013 08:06, schrieb Dan Carpenter:
> There wasn't any error handling for this kzalloc().
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
> index c06b8e5..d8293f2 100644
> --- a/drivers/scsi/osd/osd_initiator.c
> +++ b/drivers/scsi/osd/osd_initiator.c
> @@ -144,6 +144,10 @@ static int _osd_get_print_system_info(struct osd_dev *od,
> odi->osdname_len = get_attrs[a].len;
> /* Avoid NULL for memcmp optimization 0-length is good enough */
> odi->osdname = kzalloc(odi->osdname_len + 1, GFP_KERNEL);
> + if (!odi->osdname) {
> + ret = -ENOMEM;
> + goto out;
> + }
> if (odi->osdname_len)
> memcpy(odi->osdname, get_attrs[a].val_ptr, odi->osdname_len);
> OSD_INFO("OSD_NAME [%s]\n", odi->osdname);
> --
this looks like strdup() ?
re,
wh
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] [SCSI] libosd: check for kzalloc() failure
2013-01-30 8:15 ` walter harms
@ 2013-01-30 8:27 ` Dan Carpenter
2013-01-30 8:57 ` walter harms
0 siblings, 1 reply; 11+ messages in thread
From: Dan Carpenter @ 2013-01-30 8:27 UTC (permalink / raw)
To: walter harms
Cc: Boaz Harrosh, Benny Halevy, James E.J. Bottomley, osd-dev,
linux-scsi, kernel-janitors
On Wed, Jan 30, 2013 at 09:15:43AM +0100, walter harms wrote:
>
>
> Am 30.01.2013 08:06, schrieb Dan Carpenter:
> > There wasn't any error handling for this kzalloc().
> >
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >
> > diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
> > index c06b8e5..d8293f2 100644
> > --- a/drivers/scsi/osd/osd_initiator.c
> > +++ b/drivers/scsi/osd/osd_initiator.c
> > @@ -144,6 +144,10 @@ static int _osd_get_print_system_info(struct osd_dev *od,
> > odi->osdname_len = get_attrs[a].len;
> > /* Avoid NULL for memcmp optimization 0-length is good enough */
> > odi->osdname = kzalloc(odi->osdname_len + 1, GFP_KERNEL);
> > + if (!odi->osdname) {
> > + ret = -ENOMEM;
> > + goto out;
> > + }
> > if (odi->osdname_len)
> > memcpy(odi->osdname, get_attrs[a].val_ptr, odi->osdname_len);
> > OSD_INFO("OSD_NAME [%s]\n", odi->osdname);
> > --
>
> this looks like strdup() ?
>
Maybe? It's a funny thing going on with the NUL terminator and I
don't understand what the comment is about.
It appears that normally "get_attrs[a].val_ptr" is a NUL terminated
string but "get_attrs[a].len" does not count the terminator.
Odd.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] [SCSI] libosd: check for kzalloc() failure
2013-01-30 8:27 ` Dan Carpenter
@ 2013-01-30 8:57 ` walter harms
2013-01-30 9:51 ` Benny Halevy
0 siblings, 1 reply; 11+ messages in thread
From: walter harms @ 2013-01-30 8:57 UTC (permalink / raw)
To: Dan Carpenter
Cc: Boaz Harrosh, Benny Halevy, James E.J. Bottomley, osd-dev,
linux-scsi, kernel-janitors
Am 30.01.2013 09:27, schrieb Dan Carpenter:
> On Wed, Jan 30, 2013 at 09:15:43AM +0100, walter harms wrote:
>>
>>
>> Am 30.01.2013 08:06, schrieb Dan Carpenter:
>>> There wasn't any error handling for this kzalloc().
>>>
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>
>>> diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
>>> index c06b8e5..d8293f2 100644
>>> --- a/drivers/scsi/osd/osd_initiator.c
>>> +++ b/drivers/scsi/osd/osd_initiator.c
>>> @@ -144,6 +144,10 @@ static int _osd_get_print_system_info(struct osd_dev *od,
>>> odi->osdname_len = get_attrs[a].len;
>>> /* Avoid NULL for memcmp optimization 0-length is good enough */
>>> odi->osdname = kzalloc(odi->osdname_len + 1, GFP_KERNEL);
>>> + if (!odi->osdname) {
>>> + ret = -ENOMEM;
>>> + goto out;
>>> + }
>>> if (odi->osdname_len)
>>> memcpy(odi->osdname, get_attrs[a].val_ptr, odi->osdname_len);
>>> OSD_INFO("OSD_NAME [%s]\n", odi->osdname);
>>> --
>>
>> this looks like strdup() ?
>>
>
> Maybe? It's a funny thing going on with the NUL terminator and I
> don't understand what the comment is about.
>
> It appears that normally "get_attrs[a].val_ptr" is a NUL terminated
> string but "get_attrs[a].len" does not count the terminator.
>
> Odd.
>
i have no clue what the programmer was thinking. if i read this correct
osdname is u8 *osdname; so a simple strdup() or strndup() would be ok
the comment seems to indicate that get_attrs[a].val_ptr could be NULL
but where is the check ?
Perhaps they are not using ascii here ? then a memdup(get_attrs[a].len)
would be better.
re,
wh
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] [SCSI] libosd: check for kzalloc() failure
2013-01-30 8:57 ` walter harms
@ 2013-01-30 9:51 ` Benny Halevy
2013-01-30 13:00 ` walter harms
0 siblings, 1 reply; 11+ messages in thread
From: Benny Halevy @ 2013-01-30 9:51 UTC (permalink / raw)
To: wharms
Cc: Dan Carpenter, Boaz Harrosh, James E.J. Bottomley, osd-dev,
linux-scsi, kernel-janitors
On Wed, Jan 30, 2013 at 10:57 AM, walter harms <wharms@bfs.de> wrote:
>
>
> Am 30.01.2013 09:27, schrieb Dan Carpenter:
>> On Wed, Jan 30, 2013 at 09:15:43AM +0100, walter harms wrote:
>>>
>>>
>>> Am 30.01.2013 08:06, schrieb Dan Carpenter:
>>>> There wasn't any error handling for this kzalloc().
>>>>
>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>>
>>>> diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
>>>> index c06b8e5..d8293f2 100644
>>>> --- a/drivers/scsi/osd/osd_initiator.c
>>>> +++ b/drivers/scsi/osd/osd_initiator.c
>>>> @@ -144,6 +144,10 @@ static int _osd_get_print_system_info(struct osd_dev *od,
>>>> odi->osdname_len = get_attrs[a].len;
>>>> /* Avoid NULL for memcmp optimization 0-length is good enough */
>>>> odi->osdname = kzalloc(odi->osdname_len + 1, GFP_KERNEL);
>>>> + if (!odi->osdname) {
>>>> + ret = -ENOMEM;
>>>> + goto out;
>>>> + }
>>>> if (odi->osdname_len)
>>>> memcpy(odi->osdname, get_attrs[a].val_ptr, odi->osdname_len);
>>>> OSD_INFO("OSD_NAME [%s]\n", odi->osdname);
>>>> --
>>>
>>> this looks like strdup() ?
>>>
>>
>> Maybe? It's a funny thing going on with the NUL terminator and I
>> don't understand what the comment is about.
>>
>> It appears that normally "get_attrs[a].val_ptr" is a NUL terminated
>> string but "get_attrs[a].len" does not count the terminator.
>>
>> Odd.
>>
> i have no clue what the programmer was thinking. if i read this correct
> osdname is u8 *osdname; so a simple strdup() or strndup() would be ok
> the comment seems to indicate that get_attrs[a].val_ptr could be NULL
> but where is the check ?
> Perhaps they are not using ascii here ? then a memdup(get_attrs[a].len)
> would be better.
There are subtle differences between kstrdup or kmemdup and this implementation.
kmemdup is problematic as get_attrs[a].val_ptr does not contain the
NUL terminator
In the following case:
if get_attrs[a].len = 0
then get_attrs[a].val_ptr = NULL
The end result should be
odi->osdname_len = 0
odi->osdname = kzalloc(1, GFP_KERNEL);
while with kstrdup, odi->osdname will end up being NULL
as it's input arg "s" is NULL.
Benny
>
> re,
> wh
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] [SCSI] libosd: check for kzalloc() failure
2013-01-30 7:06 [patch] [SCSI] libosd: check for kzalloc() failure Dan Carpenter
2013-01-30 8:15 ` walter harms
@ 2013-01-30 10:05 ` Benny Halevy
2013-01-30 15:56 ` Boaz Harrosh
2 siblings, 0 replies; 11+ messages in thread
From: Benny Halevy @ 2013-01-30 10:05 UTC (permalink / raw)
To: Dan Carpenter
Cc: Boaz Harrosh, James E.J. Bottomley, osd-dev, linux-scsi,
kernel-janitors
On Wed, Jan 30, 2013 at 9:06 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> There wasn't any error handling for this kzalloc().
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
> index c06b8e5..d8293f2 100644
> --- a/drivers/scsi/osd/osd_initiator.c
> +++ b/drivers/scsi/osd/osd_initiator.c
> @@ -144,6 +144,10 @@ static int _osd_get_print_system_info(struct osd_dev *od,
> odi->osdname_len = get_attrs[a].len;
> /* Avoid NULL for memcmp optimization 0-length is good enough */
> odi->osdname = kzalloc(odi->osdname_len + 1, GFP_KERNEL);
> + if (!odi->osdname) {
> + ret = -ENOMEM;
> + goto out;
> + }
good catch!
Benny
> if (odi->osdname_len)
> memcpy(odi->osdname, get_attrs[a].val_ptr, odi->osdname_len);
> OSD_INFO("OSD_NAME [%s]\n", odi->osdname);
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] [SCSI] libosd: check for kzalloc() failure
2013-01-30 9:51 ` Benny Halevy
@ 2013-01-30 13:00 ` walter harms
2013-01-30 13:40 ` Benny Halevy
0 siblings, 1 reply; 11+ messages in thread
From: walter harms @ 2013-01-30 13:00 UTC (permalink / raw)
To: Benny Halevy
Cc: Dan Carpenter, Boaz Harrosh, James E.J. Bottomley, osd-dev,
linux-scsi, kernel-janitors
Am 30.01.2013 10:51, schrieb Benny Halevy:
> On Wed, Jan 30, 2013 at 10:57 AM, walter harms <wharms@bfs.de> wrote:
>>
>>
>> Am 30.01.2013 09:27, schrieb Dan Carpenter:
>>> On Wed, Jan 30, 2013 at 09:15:43AM +0100, walter harms wrote:
>>>>
>>>>
>>>> Am 30.01.2013 08:06, schrieb Dan Carpenter:
>>>>> There wasn't any error handling for this kzalloc().
>>>>>
>>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>>>
>>>>> diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
>>>>> index c06b8e5..d8293f2 100644
>>>>> --- a/drivers/scsi/osd/osd_initiator.c
>>>>> +++ b/drivers/scsi/osd/osd_initiator.c
>>>>> @@ -144,6 +144,10 @@ static int _osd_get_print_system_info(struct osd_dev *od,
>>>>> odi->osdname_len = get_attrs[a].len;
>>>>> /* Avoid NULL for memcmp optimization 0-length is good enough */
>>>>> odi->osdname = kzalloc(odi->osdname_len + 1, GFP_KERNEL);
>>>>> + if (!odi->osdname) {
>>>>> + ret = -ENOMEM;
>>>>> + goto out;
>>>>> + }
>>>>> if (odi->osdname_len)
>>>>> memcpy(odi->osdname, get_attrs[a].val_ptr, odi->osdname_len);
>>>>> OSD_INFO("OSD_NAME [%s]\n", odi->osdname);
>>>>> --
>>>>
>>>> this looks like strdup() ?
>>>>
>>>
>>> Maybe? It's a funny thing going on with the NUL terminator and I
>>> don't understand what the comment is about.
>>>
>>> It appears that normally "get_attrs[a].val_ptr" is a NUL terminated
>>> string but "get_attrs[a].len" does not count the terminator.
>>>
>>> Odd.
>>>
>> i have no clue what the programmer was thinking. if i read this correct
>> osdname is u8 *osdname; so a simple strdup() or strndup() would be ok
>> the comment seems to indicate that get_attrs[a].val_ptr could be NULL
>> but where is the check ?
>> Perhaps they are not using ascii here ? then a memdup(get_attrs[a].len)
>> would be better.
>
> There are subtle differences between kstrdup or kmemdup and this implementation.
>
> kmemdup is problematic as get_attrs[a].val_ptr does not contain the
> NUL terminator
ok, i understand - but can we assume that we are talking ascii here ?
> In the following case:
> if get_attrs[a].len = 0
> then get_attrs[a].val_ptr = NULL
>
> The end result should be
> odi->osdname_len = 0
> odi->osdname = kzalloc(1, GFP_KERNEL);
>
> while with kstrdup, odi->osdname will end up being NULL
> as it's input arg "s" is NULL.
>
and you want the argument to be "" ?
May i ask why ? kfree() can handle NULL and kprintf() (-family) also.
re,
wh
> Benny
>
>>
>> re,
>> wh
>>
>>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] [SCSI] libosd: check for kzalloc() failure
2013-01-30 13:00 ` walter harms
@ 2013-01-30 13:40 ` Benny Halevy
2013-01-30 14:34 ` walter harms
0 siblings, 1 reply; 11+ messages in thread
From: Benny Halevy @ 2013-01-30 13:40 UTC (permalink / raw)
To: wharms
Cc: Dan Carpenter, Boaz Harrosh, James E.J. Bottomley, osd-dev,
linux-scsi, kernel-janitors
On Wed, Jan 30, 2013 at 3:00 PM, walter harms <wharms@bfs.de> wrote:
>
>
> Am 30.01.2013 10:51, schrieb Benny Halevy:
>> On Wed, Jan 30, 2013 at 10:57 AM, walter harms <wharms@bfs.de> wrote:
>>>
>>>
>>> Am 30.01.2013 09:27, schrieb Dan Carpenter:
>>>> On Wed, Jan 30, 2013 at 09:15:43AM +0100, walter harms wrote:
>>>>>
>>>>>
>>>>> Am 30.01.2013 08:06, schrieb Dan Carpenter:
>>>>>> There wasn't any error handling for this kzalloc().
>>>>>>
>>>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>>>>
>>>>>> diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
>>>>>> index c06b8e5..d8293f2 100644
>>>>>> --- a/drivers/scsi/osd/osd_initiator.c
>>>>>> +++ b/drivers/scsi/osd/osd_initiator.c
>>>>>> @@ -144,6 +144,10 @@ static int _osd_get_print_system_info(struct osd_dev *od,
>>>>>> odi->osdname_len = get_attrs[a].len;
>>>>>> /* Avoid NULL for memcmp optimization 0-length is good enough */
>>>>>> odi->osdname = kzalloc(odi->osdname_len + 1, GFP_KERNEL);
>>>>>> + if (!odi->osdname) {
>>>>>> + ret = -ENOMEM;
>>>>>> + goto out;
>>>>>> + }
>>>>>> if (odi->osdname_len)
>>>>>> memcpy(odi->osdname, get_attrs[a].val_ptr, odi->osdname_len);
>>>>>> OSD_INFO("OSD_NAME [%s]\n", odi->osdname);
>>>>>> --
>>>>>
>>>>> this looks like strdup() ?
>>>>>
>>>>
>>>> Maybe? It's a funny thing going on with the NUL terminator and I
>>>> don't understand what the comment is about.
>>>>
>>>> It appears that normally "get_attrs[a].val_ptr" is a NUL terminated
>>>> string but "get_attrs[a].len" does not count the terminator.
>>>>
>>>> Odd.
>>>>
>>> i have no clue what the programmer was thinking. if i read this correct
>>> osdname is u8 *osdname; so a simple strdup() or strndup() would be ok
>>> the comment seems to indicate that get_attrs[a].val_ptr could be NULL
>>> but where is the check ?
>>> Perhaps they are not using ascii here ? then a memdup(get_attrs[a].len)
>>> would be better.
>>
>> There are subtle differences between kstrdup or kmemdup and this implementation.
>>
>> kmemdup is problematic as get_attrs[a].val_ptr does not contain the
>> NUL terminator
>
> ok, i understand - but can we assume that we are talking ascii here ?
>
No, it can be anything. UTF-8 is more likely but not guaranteed either.
>> In the following case:
>> if get_attrs[a].len = 0
>> then get_attrs[a].val_ptr = NULL
>>
>> The end result should be
>> odi->osdname_len = 0
>> odi->osdname = kzalloc(1, GFP_KERNEL);
>>
>> while with kstrdup, odi->osdname will end up being NULL
>> as it's input arg "s" is NULL.
>>
>
> and you want the argument to be "" ?
> May i ask why ? kfree() can handle NULL and kprintf() (-family) also.
It was Boaz' decision at the time to simplify internal tests
like _the_same_or_null in drivers/scsi/osd/osd_uld.c
that doesn't check for NULL
Nothing spectacular :)
Benny
>
> re,
> wh
>
>
>> Benny
>>
>>>
>>> re,
>>> wh
>>>
>>>
>>
>>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] [SCSI] libosd: check for kzalloc() failure
2013-01-30 13:40 ` Benny Halevy
@ 2013-01-30 14:34 ` walter harms
2013-01-30 15:51 ` Boaz Harrosh
0 siblings, 1 reply; 11+ messages in thread
From: walter harms @ 2013-01-30 14:34 UTC (permalink / raw)
To: Benny Halevy
Cc: Dan Carpenter, Boaz Harrosh, James E.J. Bottomley, osd-dev,
linux-scsi, kernel-janitors
Am 30.01.2013 14:40, schrieb Benny Halevy:
> On Wed, Jan 30, 2013 at 3:00 PM, walter harms <wharms@bfs.de> wrote:
>>
>>
>> Am 30.01.2013 10:51, schrieb Benny Halevy:
>>> On Wed, Jan 30, 2013 at 10:57 AM, walter harms <wharms@bfs.de> wrote:
>>>>
>>>>
>>>> Am 30.01.2013 09:27, schrieb Dan Carpenter:
>>>>> On Wed, Jan 30, 2013 at 09:15:43AM +0100, walter harms wrote:
>>>>>>
>>>>>>
>>>>>> Am 30.01.2013 08:06, schrieb Dan Carpenter:
>>>>>>> There wasn't any error handling for this kzalloc().
>>>>>>>
>>>>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>>>>>
>>>>>>> diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
>>>>>>> index c06b8e5..d8293f2 100644
>>>>>>> --- a/drivers/scsi/osd/osd_initiator.c
>>>>>>> +++ b/drivers/scsi/osd/osd_initiator.c
>>>>>>> @@ -144,6 +144,10 @@ static int _osd_get_print_system_info(struct osd_dev *od,
>>>>>>> odi->osdname_len = get_attrs[a].len;
>>>>>>> /* Avoid NULL for memcmp optimization 0-length is good enough */
>>>>>>> odi->osdname = kzalloc(odi->osdname_len + 1, GFP_KERNEL);
>>>>>>> + if (!odi->osdname) {
>>>>>>> + ret = -ENOMEM;
>>>>>>> + goto out;
>>>>>>> + }
>>>>>>> if (odi->osdname_len)
>>>>>>> memcpy(odi->osdname, get_attrs[a].val_ptr, odi->osdname_len);
>>>>>>> OSD_INFO("OSD_NAME [%s]\n", odi->osdname);
>>>>>>> --
>>>>>>
>>>>>> this looks like strdup() ?
>>>>>>
>>>>>
>>>>> Maybe? It's a funny thing going on with the NUL terminator and I
>>>>> don't understand what the comment is about.
>>>>>
>>>>> It appears that normally "get_attrs[a].val_ptr" is a NUL terminated
>>>>> string but "get_attrs[a].len" does not count the terminator.
>>>>>
>>>>> Odd.
>>>>>
>>>> i have no clue what the programmer was thinking. if i read this correct
>>>> osdname is u8 *osdname; so a simple strdup() or strndup() would be ok
>>>> the comment seems to indicate that get_attrs[a].val_ptr could be NULL
>>>> but where is the check ?
>>>> Perhaps they are not using ascii here ? then a memdup(get_attrs[a].len)
>>>> would be better.
>>>
>>> There are subtle differences between kstrdup or kmemdup and this implementation.
>>>
>>> kmemdup is problematic as get_attrs[a].val_ptr does not contain the
>>> NUL terminator
>>
>> ok, i understand - but can we assume that we are talking ascii here ?
>>
>
> No, it can be anything. UTF-8 is more likely but not guaranteed either.
>
I start to see the complexity of the situation. Would you mind to add
the comment "it can be anything. UTF-8 is more likely but not guaranteed either" ?
For now using a pascal-string seems the best solution but it should be warned
that get_attrs[a].val_ptr is NOT a c-string and therefore can not be used with
the printf-family (i guess the situation will become more clear in future)
I have no clue why you need that, using c-strings makes life more easy in the
last minute a sprintf(buf,"%u") will save the day ;)
>>> In the following case:
>>> if get_attrs[a].len = 0
>>> then get_attrs[a].val_ptr = NULL
>>>
>>> The end result should be
>>> odi->osdname_len = 0
>>> odi->osdname = kzalloc(1, GFP_KERNEL);
>>>
>>> while with kstrdup, odi->osdname will end up being NULL
>>> as it's input arg "s" is NULL.
>>>
>>
>> and you want the argument to be "" ?
>> May i ask why ? kfree() can handle NULL and kprintf() (-family) also.
>
> It was Boaz' decision at the time to simplify internal tests
> like _the_same_or_null in drivers/scsi/osd/osd_uld.c
> that doesn't check for NULL
>
It look clever to add the NULL test here.
Noone reviewing the code will understand that.
(Rule of least surprise)
just my 2 cents,
re,
wh
> Nothing spectacular :)
>
> Benny
>
>>
>> re,
>> wh
>>
>>
>>> Benny
>>>
>>>>
>>>> re,
>>>> wh
>>>>
>>>>
>>>
>>>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] [SCSI] libosd: check for kzalloc() failure
2013-01-30 14:34 ` walter harms
@ 2013-01-30 15:51 ` Boaz Harrosh
0 siblings, 0 replies; 11+ messages in thread
From: Boaz Harrosh @ 2013-01-30 15:51 UTC (permalink / raw)
To: wharms
Cc: Benny Halevy, Dan Carpenter, James E.J. Bottomley, osd-dev,
linux-scsi, kernel-janitors
On 01/30/2013 04:34 PM, walter harms wrote:
>
<>
> I start to see the complexity of the situation. Would you mind to add
> the comment "it can be anything. UTF-8 is more likely but not guaranteed either" ?
> For now using a pascal-string seems the best solution but it should be warned
> that get_attrs[a].val_ptr is NOT a c-string and therefore can not be used with
> the printf-family (i guess the situation will become more clear in future)
>
OK, So... The long story
The STD says that osdname can be *anything* binary or otherwise, and of *any*
length. And is used to uniquely identify the OSD-device/partition in a cluster.
We decided that if so we would stick an 40 char UUID in it, generated by a uuidgen
and all the external utilities and surrounding code forces it, and treat it as
a c-string. But this code here in the core cannot make that assumption and still
support the STD.
On the other hand we did want the osdname to be printable in traces and messages
because it is such an important identifier. So I have decided to sacrifice an
extra char in-memory to carry a \NUL and safely stick it into printk(s). Those
(none existent) OSD devices that will put unprintable characters in here will
still function fine, but will look real scary in printk(s).
Please note that the one that sets policy is the osd-target vendor. (And they
all currently use my code base)
So save the kzalloc check this code (tested) is safe and will show strings when
present, but will gracefully show ugly things but still work when not.
> I have no clue why you need that, using c-strings makes life more easy in the
> last minute a sprintf(buf,"%u") will save the day ;)
>
Actually it is very funny, because just recently (last week) I have discovered
something that eliminates all those funny business.
printf("%*s", odi->osdname, odi->osdname_len);
The "*" will instruct c to expect an extra variable following %s which is the
max_length of %s. This is exactly for pascal strings and the such like above.
So I added a TODO to clean that a bit by always printing with "%*s"
>>
> It look clever to add the NULL test here.
> Noone reviewing the code will understand that.
> (Rule of least surprise)
>
Thanks for looking, I agree it needs a fat comment, I'll do that when I'll
convert to above system. Thanks for looking, That code is really old and never
had any extra eyes on it.
> just my 2 cents,
> re,
> wh
>
Cheers
Boaz
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [patch] [SCSI] libosd: check for kzalloc() failure
2013-01-30 7:06 [patch] [SCSI] libosd: check for kzalloc() failure Dan Carpenter
2013-01-30 8:15 ` walter harms
2013-01-30 10:05 ` Benny Halevy
@ 2013-01-30 15:56 ` Boaz Harrosh
2 siblings, 0 replies; 11+ messages in thread
From: Boaz Harrosh @ 2013-01-30 15:56 UTC (permalink / raw)
To: Dan Carpenter, James E.J. Bottomley
Cc: Benny Halevy, osd-dev, linux-scsi, kernel-janitors
On 01/30/2013 09:06 AM, Dan Carpenter wrote:
> There wasn't any error handling for this kzalloc().
>
ACK-by: Boaz Harrosh <bharrosh@panasas.com>
James please queue for inclusion
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Thanks Dan
>
> diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
> index c06b8e5..d8293f2 100644
> --- a/drivers/scsi/osd/osd_initiator.c
> +++ b/drivers/scsi/osd/osd_initiator.c
> @@ -144,6 +144,10 @@ static int _osd_get_print_system_info(struct osd_dev *od,
> odi->osdname_len = get_attrs[a].len;
> /* Avoid NULL for memcmp optimization 0-length is good enough */
> odi->osdname = kzalloc(odi->osdname_len + 1, GFP_KERNEL);
> + if (!odi->osdname) {
> + ret = -ENOMEM;
> + goto out;
> + }
> if (odi->osdname_len)
> memcpy(odi->osdname, get_attrs[a].val_ptr, odi->osdname_len);
> OSD_INFO("OSD_NAME [%s]\n", odi->osdname);
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-01-30 15:56 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-01-30 7:06 [patch] [SCSI] libosd: check for kzalloc() failure Dan Carpenter
2013-01-30 8:15 ` walter harms
2013-01-30 8:27 ` Dan Carpenter
2013-01-30 8:57 ` walter harms
2013-01-30 9:51 ` Benny Halevy
2013-01-30 13:00 ` walter harms
2013-01-30 13:40 ` Benny Halevy
2013-01-30 14:34 ` walter harms
2013-01-30 15:51 ` Boaz Harrosh
2013-01-30 10:05 ` Benny Halevy
2013-01-30 15:56 ` Boaz Harrosh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox