* [PATCH] ulogd: add +1 char for null char
@ 2017-03-20 8:31 Alexandru Ardelean
2017-03-21 20:54 ` Eric Leblond
0 siblings, 1 reply; 5+ messages in thread
From: Alexandru Ardelean @ 2017-03-20 8:31 UTC (permalink / raw
To: netfilter-devel; +Cc: eric, Alexandru Ardelean
This is a bit zealous to fix like this, but it seems to work.
The crash was reproduced on ppc32, with GCC 5.4 & musl libc 1.1.16.
And also on LEDE (mips_24kc and ARM):
https://github.com/openwrt/packages/issues/4123
https://github.com/openwrt/packages/issues/4090
I personally saw it on ppc32.
The offending code was in `pluginstance_alloc_init()` line 671:
```
memcpy(pi->id, pi_id, sizeof(pi->id));
```
Seems that it would copy 1 char from the stack, and that
caused some failsafes to kick in.
This fix addresses the issue directly.
Maybe a more appropriate rework of string stuff would be needed.
What I also noticed, is that there's also places in the code
that define name[ULOGD_MAX_KEYLEN+1] and some that don't add
the +1 char.
Basically, this just aligns the remaining bits of code
that don't add the +1 char.
Signed-off-by: Alexandru Ardelean <ardeleanalex@gmail.com>
---
output/sqlite3/ulogd_output_SQLITE3.c | 6 +++---
src/ulogd.c | 2 +-
util/db.c | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/output/sqlite3/ulogd_output_SQLITE3.c b/output/sqlite3/ulogd_output_SQLITE3.c
index 20ceb3b..ea66061 100644
--- a/output/sqlite3/ulogd_output_SQLITE3.c
+++ b/output/sqlite3/ulogd_output_SQLITE3.c
@@ -48,7 +48,7 @@
struct field {
TAILQ_ENTRY(field) link;
- char name[ULOGD_MAX_KEYLEN];
+ char name[ULOGD_MAX_KEYLEN+1]; /* +1 for null char */
struct ulogd_key *key;
};
@@ -214,7 +214,7 @@ sqlite3_createstmt(struct ulogd_pluginstance *pi)
{
struct sqlite3_priv *priv = (void *)pi->private;
struct field *f;
- char buf[ULOGD_MAX_KEYLEN];
+ char buf[ULOGD_MAX_KEYLEN+1]; /* +1 for null char */
char *underscore;
char *stmt_pos;
int i, cols = 0;
@@ -305,7 +305,7 @@ static int
sqlite3_init_db(struct ulogd_pluginstance *pi)
{
struct sqlite3_priv *priv = (void *)pi->private;
- char buf[ULOGD_MAX_KEYLEN];
+ char buf[ULOGD_MAX_KEYLEN+1];
char *underscore;
struct field *f;
sqlite3_stmt *schema_stmt;
diff --git a/src/ulogd.c b/src/ulogd.c
index 5b9a586..0d6a367 100644
--- a/src/ulogd.c
+++ b/src/ulogd.c
@@ -942,7 +942,7 @@ static int create_stack(const char *option)
/* PASS 1: find and instanciate plugins of stack, link them together */
for (tok = strtok(buf, ",\n"); tok; tok = strtok(NULL, ",\n")) {
char *plname, *equals;
- char pi_id[ULOGD_MAX_KEYLEN];
+ char pi_id[ULOGD_MAX_KEYLEN+1]; /* +1 for the null char */
struct ulogd_pluginstance *pi;
struct ulogd_plugin *pl;
diff --git a/util/db.c b/util/db.c
index c9aec41..6af4555 100644
--- a/util/db.c
+++ b/util/db.c
@@ -96,7 +96,7 @@ static int sql_createstmt(struct ulogd_pluginstance *upi)
if (strncasecmp(procedure,"INSERT", strlen("INSERT")) == 0 &&
(procedure[strlen("INSERT")] == '\0' ||
procedure[strlen("INSERT")] == ' ')) {
- char buf[ULOGD_MAX_KEYLEN];
+ char buf[ULOGD_MAX_KEYLEN+1]; /* +1 for null char */
char *underscore;
if(procedure[6] == '\0') {
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ulogd: add +1 char for null char
2017-03-20 8:31 [PATCH] ulogd: add +1 char for null char Alexandru Ardelean
@ 2017-03-21 20:54 ` Eric Leblond
2017-03-21 20:56 ` [PATCH] ulogd: use strncpy instead of memcpy Eric Leblond
2017-03-22 7:07 ` [PATCH] ulogd: add +1 char for null char Alexandru Ardelean
0 siblings, 2 replies; 5+ messages in thread
From: Eric Leblond @ 2017-03-21 20:54 UTC (permalink / raw
To: Alexandru Ardelean, netfilter-devel
Hello,
Thanks for the report and the patch. I'm not sure of your
implementation. Can you test with the patch to follow ?
On Mon, 2017-03-20 at 10:31 +0200, Alexandru Ardelean wrote:
> This is a bit zealous to fix like this, but it seems to work.
>
> The crash was reproduced on ppc32, with GCC 5.4 & musl libc 1.1.16.
>
> And also on LEDE (mips_24kc and ARM):
> https://github.com/openwrt/packages/issues/4123
> https://github.com/openwrt/packages/issues/4090
>
> I personally saw it on ppc32.
> The offending code was in `pluginstance_alloc_init()` line 671:
> ```
> memcpy(pi->id, pi_id, sizeof(pi->id));
> ```
Thanks in advance,
--
Eric Leblond <eric@regit.org>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] ulogd: use strncpy instead of memcpy
2017-03-21 20:54 ` Eric Leblond
@ 2017-03-21 20:56 ` Eric Leblond
2017-03-22 7:07 ` [PATCH] ulogd: add +1 char for null char Alexandru Ardelean
1 sibling, 0 replies; 5+ messages in thread
From: Eric Leblond @ 2017-03-21 20:56 UTC (permalink / raw
To: Alexandru Ardelean; +Cc: netfilter-devel, Eric Leblond
On some architecture, ulogd is not starting due to a
crash in memcpy. This patch switches to strncpy to
avoid the problem.
Reported-by: Alexandru Ardelean <ardeleanalex@gmail.com>
Signed-off-by: Eric Leblond <eric@regit.org>
---
src/ulogd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/ulogd.c b/src/ulogd.c
index 5b9a586..919a317 100644
--- a/src/ulogd.c
+++ b/src/ulogd.c
@@ -668,7 +668,7 @@ pluginstance_alloc_init(struct ulogd_plugin *pl, char *pi_id,
INIT_LLIST_HEAD(&pi->plist);
pi->plugin = pl;
pi->stack = stack;
- memcpy(pi->id, pi_id, sizeof(pi->id));
+ strncpy(pi->id, pi_id, ULOGD_MAX_KEYLEN);
ptr = (void *)pi + sizeof(*pi);
--
2.11.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ulogd: add +1 char for null char
2017-03-21 20:54 ` Eric Leblond
2017-03-21 20:56 ` [PATCH] ulogd: use strncpy instead of memcpy Eric Leblond
@ 2017-03-22 7:07 ` Alexandru Ardelean
2017-03-22 15:56 ` Alexandru Ardelean
1 sibling, 1 reply; 5+ messages in thread
From: Alexandru Ardelean @ 2017-03-22 7:07 UTC (permalink / raw
To: Eric Leblond; +Cc: netfilter-devel
On Tue, Mar 21, 2017 at 10:54 PM, Eric Leblond <eric@regit.org> wrote:
> Hello,
>
> Thanks for the report and the patch. I'm not sure of your
> implementation. Can you test with the patch to follow ?
>
> On Mon, 2017-03-20 at 10:31 +0200, Alexandru Ardelean wrote:
>> This is a bit zealous to fix like this, but it seems to work.
>>
>> The crash was reproduced on ppc32, with GCC 5.4 & musl libc 1.1.16.
>>
>> And also on LEDE (mips_24kc and ARM):
>> https://github.com/openwrt/packages/issues/4123
>> https://github.com/openwrt/packages/issues/4090
>>
>> I personally saw it on ppc32.
>> The offending code was in `pluginstance_alloc_init()` line 671:
>> ```
>> memcpy(pi->id, pi_id, sizeof(pi->id));
>> ```
>
> Thanks in advance,
> --
> Eric Leblond <eric@regit.org>
Hey,
Thanks for the response.
Will test out your patch & come back.
After submitting mine, there was a similar discussion on one of the
Github threads [to use strncpy()/strlcpy()].
A few questions:
1) would strlcpy(d,s,len) be better [here & in general] ? [since it
guarantees a null-char at the end of `len`] ?
1a) maybe it could be considered to replace strncpy() in more places
[where the case is appropriate]
2) any thoughts on sanitizing the use of ULOGD_MAX_KEYLEN ? ; general
gist of it would be #define ULOGD_MAX_KEYLEN 32 and remove any
`ULOGD_MAX_KEYLEN+1` or `ULOGD_MAX_KEYLEN-1` ; which sort of seemed
confusing ; and combined with strlcpy() it should give an overall more
robust approach
Thanks
Alex
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ulogd: add +1 char for null char
2017-03-22 7:07 ` [PATCH] ulogd: add +1 char for null char Alexandru Ardelean
@ 2017-03-22 15:56 ` Alexandru Ardelean
0 siblings, 0 replies; 5+ messages in thread
From: Alexandru Ardelean @ 2017-03-22 15:56 UTC (permalink / raw
To: Eric Leblond; +Cc: netfilter-devel
On Wed, Mar 22, 2017 at 9:07 AM, Alexandru Ardelean
<ardeleanalex@gmail.com> wrote:
> On Tue, Mar 21, 2017 at 10:54 PM, Eric Leblond <eric@regit.org> wrote:
>> Hello,
>>
>> Thanks for the report and the patch. I'm not sure of your
>> implementation. Can you test with the patch to follow ?
>>
>> On Mon, 2017-03-20 at 10:31 +0200, Alexandru Ardelean wrote:
>>> This is a bit zealous to fix like this, but it seems to work.
>>>
>>> The crash was reproduced on ppc32, with GCC 5.4 & musl libc 1.1.16.
>>>
>>> And also on LEDE (mips_24kc and ARM):
>>> https://github.com/openwrt/packages/issues/4123
>>> https://github.com/openwrt/packages/issues/4090
>>>
>>> I personally saw it on ppc32.
>>> The offending code was in `pluginstance_alloc_init()` line 671:
>>> ```
>>> memcpy(pi->id, pi_id, sizeof(pi->id));
>>> ```
>>
>> Thanks in advance,
>> --
>> Eric Leblond <eric@regit.org>
>
> Hey,
>
> Thanks for the response.
> Will test out your patch & come back.
>
> After submitting mine, there was a similar discussion on one of the
> Github threads [to use strncpy()/strlcpy()].
>
> A few questions:
> 1) would strlcpy(d,s,len) be better [here & in general] ? [since it
> guarantees a null-char at the end of `len`] ?
> 1a) maybe it could be considered to replace strncpy() in more places
> [where the case is appropriate]
> 2) any thoughts on sanitizing the use of ULOGD_MAX_KEYLEN ? ; general
> gist of it would be #define ULOGD_MAX_KEYLEN 32 and remove any
> `ULOGD_MAX_KEYLEN+1` or `ULOGD_MAX_KEYLEN-1` ; which sort of seemed
> confusing ; and combined with strlcpy() it should give an overall more
> robust approach
>
> Thanks
> Alex
on a more punctual note
your patch works :)
feel free to push
regarding my comments, i don't have strong opinions ; i'll leave it up to you
thanks
Alex
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-03-22 15:56 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-20 8:31 [PATCH] ulogd: add +1 char for null char Alexandru Ardelean
2017-03-21 20:54 ` Eric Leblond
2017-03-21 20:56 ` [PATCH] ulogd: use strncpy instead of memcpy Eric Leblond
2017-03-22 7:07 ` [PATCH] ulogd: add +1 char for null char Alexandru Ardelean
2017-03-22 15:56 ` Alexandru Ardelean
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.