This patch was not yet tested. Please confirm it's right.
---
strncpy does not append '\0' if the length of the source string equals
the size parameter, strlcpy does.
Signed-off-by: Roel Kluin <[email protected]>
---
diff --git a/arch/arm/common/locomo.c b/arch/arm/common/locomo.c
index ae21755..89b2328 100644
--- a/arch/arm/common/locomo.c
+++ b/arch/arm/common/locomo.c
@@ -516,7 +516,7 @@ locomo_init_one_child(struct locomo *lchip, struct locomo_dev_info *info)
goto out;
}
- strncpy(dev->dev.bus_id, info->name, sizeof(dev->dev.bus_id));
+ strlcpy(dev->dev.bus_id, info->name, sizeof(dev->dev.bus_id));
/*
* If the parent device has a DMA mask associated with it,
* propagate it down to the children.
Roel Kluin wrote:
> This patch was not yet tested. Please confirm it's right.
> ---
> strncpy does not append '\0' if the length of the source string equals
> the size parameter, strlcpy does.
>
Are you sure it's safe to not zero out the contents of the buffer (no
information leak)?
-hpa
H. Peter Anvin wrote:
> Roel Kluin wrote:
>> This patch was not yet tested. Please confirm it's right.
>> ---
>> strncpy does not append '\0' if the length of the source string equals
>> the size parameter, strlcpy does.
>>
>
> Are you sure it's safe to not zero out the contents of the buffer (no
> information leak)?
>
> -hpa
As I understand it, please correct me if I'm wrong:
Of the three variants: strcpy, strncpy and strlcpy.
- strcpy does not append \0 (unless the source string already contained it)
- strncpy appends \0's if the source string is smaller than the size
parameter (for all remaining characters)
- strlcpy always appends a single \0 (unless size parameter was 0)
char *strcpy(char *dest, const char *src);
char *strncpy(char *dest, const char *src, size_t n);
size_t strlcpy(char *dst, const char *src, size_t n);
In the original code strncpy was used and the size parameter was equal
to the source string size:
strncpy(dev->dev.bus_id, info->name, sizeof(dev->dev.bus_id));
Since this the size was equal there was no \0 termination. To \0
terminate using strncpy we could write:
strncpy(dev->dev.bus_id, info->name, sizeof(dev->dev.bus_id) - 1);
dev->dev.bus_id[sizeof(dev->dev.bus_id) - 1] = '\0';
or using strlcpy, which does the same thing:
strlcpy(dev->dev.bus_id, info->name, sizeof(dev->dev.bus_id));
Roel
Roel Kluin wrote:
>As I understand it, please correct me if I'm wrong:
>
>Of the three variants: strcpy, strncpy and strlcpy.
>- strcpy does not append \0 (unless the source string already contained it)
No, that's not correct. If the source string has no \0, then
strcpy keeps reading forever and things go horribly awry.
>In the original code strncpy was used and the size parameter was equal
>to the source string size:
>
>strncpy(dev->dev.bus_id, info->name, sizeof(dev->dev.bus_id));
>
>Since this the size was equal there was no \0 termination. To \0
>terminate using strncpy we could write:
>
>strncpy(dev->dev.bus_id, info->name, sizeof(dev->dev.bus_id) - 1);
>dev->dev.bus_id[sizeof(dev->dev.bus_id) - 1] = '\0';
>
>or using strlcpy, which does the same thing:
>
>strlcpy(dev->dev.bus_id, info->name, sizeof(dev->dev.bus_id));
No, strlcpy() does not do the same thing. As several people have pointed
out to you, strlcpy() does not fill the rest of the buffer with \0's.
This can create an information leak in some cases, which can create a
security hole. The difference is important in some cases.
Second, you seem to implicitly assume that it's a bug to leave
dev->dev.bus_id without any \0 termination. You'd have to look at the API
to determine whether that assumption is accurate. And then you'd have to
think carefully about what is the proper behavior if info->name is too
long to fit into the buffer. Is truncating the string the appropriate
behavior? In some cases prematurely truncating a string is bad news and
can create a security bug, and the proper thing to do in some cases may
be to return an error rather than returning a truncated string.
Rant: Blindly search-and-replacing strncpy() with strlcpy() is not safe,
in general. You have to actually understand what the code is doing and
what it should be doing.
I'm not claiming that your patch is correct, nor that it is incorrect.
Instead, I'm suggesting that you probably need to do some analysis to
understand the specific requirements and context in each of these cases,
rather than doing a mechanically search-and-replace.