[pacman-dev] [PATCH] package.c, fix incorrect buffersize

newer
[pacman-dev] [PATCH v2] package.c:...

older
[pacman-dev] [PATCH] add detail to...

Rikard Falkeborn

1 Nov 2015 1 Nov '15

12:32 a.m.

Correct title_suffix_len to be the actual number of elements in the string (including the NUL-terminator) instead of the size of a pointer. Note that wmemcpy blindly copies the number of wide characters it is told to copy (no check for NUL-terminating character), so this previously copied data outside of title_suffix. Signed-off-by: Rikard Falkeborn --- src/pacman/package.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/pacman/package.c b/src/pacman/package.c index dbd23f5..59f4327 100644 --- a/src/pacman/package.c +++ b/src/pacman/package.c @@ -79,6 +79,8 @@ enum { * potential growth. */ #define TITLE_MAXLEN 50 +#define TITLE_SUFFIX L" :" +#define TITLE_SUFFIX_LEN (ARRAYSIZE(TITLE_SUFFIX)) static char titles[_T_MAX][TITLE_MAXLEN * sizeof(wchar_t)]; @@ -90,9 +92,7 @@ static void make_aligned_titles(void) { unsigned int i; size_t max = 0; - static const wchar_t *title_suffix = L" :"; - static const size_t title_suffix_len = sizeof(title_suffix); - wchar_t wbuf[ARRAYSIZE(titles)][TITLE_MAXLEN + title_suffix_len]; + wchar_t wbuf[ARRAYSIZE(titles)][TITLE_MAXLEN + TITLE_SUFFIX_LEN]; size_t wlen[ARRAYSIZE(wbuf)]; char *buf[ARRAYSIZE(wbuf)]; buf[T_ARCHITECTURE] = _("Architecture"); @@ -133,7 +133,7 @@ static void make_aligned_titles(void) for(i = 0; i < ARRAYSIZE(wbuf); i++) { wmemset(wbuf[i] + wlen[i], L' ', max - wlen[i]); - wmemcpy(wbuf[i] + max, title_suffix, title_suffix_len); + wmemcpy(wbuf[i] + max, TITLE_SUFFIX, TITLE_SUFFIX_LEN); wcstombs(titles[i], wbuf[i], sizeof(wbuf[i])); } } -- 2.6.2

Show replies by date

Pierre Neidhardt

1 Nov 1 Nov

8:35 a.m.

There is a much simpler fix:

...

- static const wchar_t *title_suffix = L" :"; - static const wchar_t title_suffix[] = L" :";

I'll commit the patch right row. -- Pierre Neidhardt To communicate is the beginning of understanding. -- AT&T

Pierre Neidhardt

8:38 a.m.

Unless you want to do it of course :) -- Pierre Neidhardt Knowledge without common sense is folly.

Rikard Falkeborn

8:55 a.m.

Den 1 nov 2015 09:36 skrev "Pierre Neidhardt" :

...

There is a much simpler fix:

...
- static const wchar_t *title_suffix = L" :"; - static const wchar_t title_suffix[] = L" :";

I'll commit the patch right row.

-- Pierre Neidhardt

To communicate is the beginning of understanding. -- AT&T

That's simpler. :) Can you check if clang is happy with the fix? The reason I found it in the first place was a clang warning. I don't have a computer nearby to test it on myself at the moment. /Rikard

Rikard Falkeborn

9:05 a.m.

Den 1 nov 2015 09:55 skrev "Rikard Falkeborn" :

...

Den 1 nov 2015 09:36 skrev "Pierre Neidhardt" :

...
There is a much simpler fix:

...
- static const wchar_t *title_suffix = L" :"; - static const wchar_t title_suffix[] = L" :";

I'll commit the patch right row.

-- Pierre Neidhardt

To communicate is the beginning of understanding. -- AT&T

That's simpler. :)

Can you check if clang is happy with the fix? The reason I found it in

the first place was a clang warning. I don't have a computer nearby to test it on myself at the moment.

...

/Rikard

Simpler but not correct I think. We don't want the size in bytes, we want the number of elements. So making it an array and using ARRAYSIZE would do the trick. Provided clang doesn't warn... :) Rikard

Pierre Neidhardt

9:39 a.m.

On 15-11-01 10:05:30, Rikard Falkeborn wrote:

...

Den 1 nov 2015 09:55 skrev "Rikard Falkeborn" :

...
Den 1 nov 2015 09:36 skrev "Pierre Neidhardt" :

...
There is a much simpler fix:

...
- static const wchar_t *title_suffix = L" :"; - static const wchar_t title_suffix[] = L" :";

I'll commit the patch right row.

-- Pierre Neidhardt

To communicate is the beginning of understanding. -- AT&T

That's simpler. :)

Can you check if clang is happy with the fix? The reason I found it in

the first place was a clang warning. I don't have a computer nearby to test it on myself at the moment.

...
/Rikard

Simpler but not correct I think. We don't want the size in bytes, we want the number of elements. So making it an array and using ARRAYSIZE would do the trick. Provided clang doesn't warn... :)

Indeed. Replace the 'sizeof' right after with ARRAYSIZE. Good catch! -- Pierre Neidhardt

Pierre Neidhardt

9:33 a.m.

This is ANSI C: `sizeof` returns the size of the object in bytes. When the argument is a static array, this is one of the exceptions to the rule that the `array of T` is automatically converted to a `pointer to T`. Thus `sizeof` will return the size in bytes of the entire array. See _The C Programming Language_, some C draft out there, or https://en.wikipedia.org/wiki/Sizeof. -- Pierre Neidhardt cursor address, n: "Hello, cursor!" -- Stan Kelly-Bootle, "The Devil's DP Dictionary"

Allan McRae

9:50 a.m.

On 01/11/15 10:32, Rikard Falkeborn wrote:

...

Correct title_suffix_len to be the actual number of elements in the string (including the NUL-terminator) instead of the size of a pointer.

Note that wmemcpy blindly copies the number of wide characters it is told to copy (no check for NUL-terminating character), so this previously copied data outside of title_suffix.

This is actually not the cause of the warning from clang... But it does fix it (unlike all the other suggestions in the thread). package.c:95:34: error: variable length array folded to constant array as an extension [-Werror,-Wgnu-folding-constant] wchar_t wbuf[ARRAYSIZE(titles)][TITLE_MAXLEN + title_suffix_len]; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ package.c:81:22: note: expanded from macro 'TITLE_MAXLEN' #define TITLE_MAXLEN 50 ^

...

Signed-off-by: Rikard Falkeborn --- src/pacman/package.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/pacman/package.c b/src/pacman/package.c index dbd23f5..59f4327 100644 --- a/src/pacman/package.c +++ b/src/pacman/package.c @@ -79,6 +79,8 @@ enum { * potential growth. */ #define TITLE_MAXLEN 50 +#define TITLE_SUFFIX L" :" +#define TITLE_SUFFIX_LEN (ARRAYSIZE(TITLE_SUFFIX))

static char titles[_T_MAX][TITLE_MAXLEN * sizeof(wchar_t)];

@@ -90,9 +92,7 @@ static void make_aligned_titles(void) { unsigned int i; size_t max = 0; - static const wchar_t *title_suffix = L" :"; - static const size_t title_suffix_len = sizeof(title_suffix); - wchar_t wbuf[ARRAYSIZE(titles)][TITLE_MAXLEN + title_suffix_len]; + wchar_t wbuf[ARRAYSIZE(titles)][TITLE_MAXLEN + TITLE_SUFFIX_LEN]; size_t wlen[ARRAYSIZE(wbuf)]; char *buf[ARRAYSIZE(wbuf)]; buf[T_ARCHITECTURE] = _("Architecture"); @@ -133,7 +133,7 @@ static void make_aligned_titles(void)

for(i = 0; i < ARRAYSIZE(wbuf); i++) { wmemset(wbuf[i] + wlen[i], L' ', max - wlen[i]); - wmemcpy(wbuf[i] + max, title_suffix, title_suffix_len); + wmemcpy(wbuf[i] + max, TITLE_SUFFIX, TITLE_SUFFIX_LEN); wcstombs(titles[i], wbuf[i], sizeof(wbuf[i])); } }

Pierre Neidhardt

10:23 a.m.

On 15-11-01 19:50:56, Allan McRae wrote:

...

On 01/11/15 10:32, Rikard Falkeborn wrote:

...
Correct title_suffix_len to be the actual number of elements in the string (including the NUL-terminator) instead of the size of a pointer.

Note that wmemcpy blindly copies the number of wide characters it is told to copy (no check for NUL-terminating character), so this previously copied data outside of title_suffix.

This is actually not the cause of the warning from clang... But it does fix it (unlike all the other suggestions in the thread).

package.c:95:34: error: variable length array folded to constant array as an extension [-Werror,-Wgnu-folding-constant] wchar_t wbuf[ARRAYSIZE(titles)][TITLE_MAXLEN + title_suffix_len]; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ package.c:81:22: note: expanded from macro 'TITLE_MAXLEN' #define TITLE_MAXLEN 50 ^

Right, there were two errors here. One was the memory overflow pointed out by Rikard. If I'm not mistaken, the second one reported by clang comes from that 'const' variables are not constants semantically speaking, they are immutable. And a constant array must be initialized with constant size(s). Crap. :( Simpler fix would be: - static const wchar_t title_suffix[] = L" :"; - static const size_t title_suffix_len = sizeof(title_suffix); - wchar_t wbuf[ARRAYSIZE(titles)][TITLE_MAXLEN + title_suffix_len]; + static const wchar_t title_suffix[] = L" :"; + wchar_t wbuf[ARRAYSIZE(titles)][TITLE_MAXLEN + ARRAYSIZE(title_suffix)]; ... - wmemcpy(wbuf[i] + max, title_suffix, title_suffix_len); + wmemcpy(wbuf[i] + max, title_suffix, ARRAYSIZE(title_suffix)); -- Pierre Neidhardt

Dave Reisner

12:09 p.m.

On Sun, Nov 01, 2015 at 01:32:59AM +0100, Rikard Falkeborn wrote:

...

Correct title_suffix_len to be the actual number of elements in the string (including the NUL-terminator) instead of the size of a pointer.

Note that wmemcpy blindly copies the number of wide characters it is told to copy (no check for NUL-terminating character), so this previously copied data outside of title_suffix.

Signed-off-by: Rikard Falkeborn ---

Just a thought -- instead of dealing with calculating fixed sized buffers, why don't we just calculate the max width needed, and then use printf to do the actual formatting? It prevents two whole classes of problems: buffer overflows and potential string truncation. The patch is somewhat insidious as it requires modifying list_display, string_display, etc., but I've already got a mostly working POC already.

...

src/pacman/package.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/pacman/package.c b/src/pacman/package.c index dbd23f5..59f4327 100644 --- a/src/pacman/package.c +++ b/src/pacman/package.c @@ -79,6 +79,8 @@ enum { * potential growth. */ #define TITLE_MAXLEN 50 +#define TITLE_SUFFIX L" :" +#define TITLE_SUFFIX_LEN (ARRAYSIZE(TITLE_SUFFIX))

static char titles[_T_MAX][TITLE_MAXLEN * sizeof(wchar_t)];

@@ -90,9 +92,7 @@ static void make_aligned_titles(void) { unsigned int i; size_t max = 0; - static const wchar_t *title_suffix = L" :"; - static const size_t title_suffix_len = sizeof(title_suffix); - wchar_t wbuf[ARRAYSIZE(titles)][TITLE_MAXLEN + title_suffix_len]; + wchar_t wbuf[ARRAYSIZE(titles)][TITLE_MAXLEN + TITLE_SUFFIX_LEN]; size_t wlen[ARRAYSIZE(wbuf)]; char *buf[ARRAYSIZE(wbuf)]; buf[T_ARCHITECTURE] = _("Architecture"); @@ -133,7 +133,7 @@ static void make_aligned_titles(void)

for(i = 0; i < ARRAYSIZE(wbuf); i++) { wmemset(wbuf[i] + wlen[i], L' ', max - wlen[i]); - wmemcpy(wbuf[i] + max, title_suffix, title_suffix_len); + wmemcpy(wbuf[i] + max, TITLE_SUFFIX, TITLE_SUFFIX_LEN); wcstombs(titles[i], wbuf[i], sizeof(wbuf[i])); } } -- 2.6.2

Pierre Neidhardt

1:09 p.m.

On 15-11-01 07:09:30, Dave Reisner wrote:

...

On Sun, Nov 01, 2015 at 01:32:59AM +0100, Rikard Falkeborn wrote:

...
Correct title_suffix_len to be the actual number of elements in the string (including the NUL-terminator) instead of the size of a pointer.

Note that wmemcpy blindly copies the number of wide characters it is told to copy (no check for NUL-terminating character), so this previously copied data outside of title_suffix.

Signed-off-by: Rikard Falkeborn ---

Just a thought -- instead of dealing with calculating fixed sized buffers, why don't we just calculate the max width needed, and then use printf to do the actual formatting? It prevents two whole classes of problems: buffer overflows and potential string truncation.

The patch is somewhat insidious as it requires modifying list_display, string_display, etc., but I've already got a mostly working POC already.

I like it. More insidious, but I think the pros overweight the cons. Besides it would make alignment straighforward to add to other places if need be in the future. Just in case, you'll need to use wprintf on every print where alignment is needed. -- Pierre Neidhardt

Allan McRae

2 Nov 2 Nov

4:20 a.m.

On 01/11/15 23:09, Pierre Neidhardt wrote:

...

On 15-11-01 07:09:30, Dave Reisner wrote:

...
On Sun, Nov 01, 2015 at 01:32:59AM +0100, Rikard Falkeborn wrote:

...
Correct title_suffix_len to be the actual number of elements in the string (including the NUL-terminator) instead of the size of a pointer.

Note that wmemcpy blindly copies the number of wide characters it is told to copy (no check for NUL-terminating character), so this previously copied data outside of title_suffix.

Signed-off-by: Rikard Falkeborn ---

Just a thought -- instead of dealing with calculating fixed sized buffers, why don't we just calculate the max width needed, and then use printf to do the actual formatting? It prevents two whole classes of problems: buffer overflows and potential string truncation.

The patch is somewhat insidious as it requires modifying list_display, string_display, etc., but I've already got a mostly working POC already.

I like it. More insidious, but I think the pros overweight the cons. Besides it would make alignment straighforward to add to other places if need be in the future.

Just in case, you'll need to use wprintf on every print where alignment is needed.

Can I get a patch with just the fix to this issue (i.e. what Pierre sent in an earlier email)? Any change to using (w)printf can occur on top of that. Thanks, Allan

Pierre Neidhardt

7:46 a.m.

Since you caught the issue, Rikard, do you want to do send the patch? -- Pierre Neidhardt

Rikard Falkeborn

10:49 a.m.

Den 2 nov 2015 08:47 skrev "Pierre Neidhardt" :

...

Since you caught the issue, Rikard, do you want to do send the patch?

-- Pierre Neidhardt

Was there a problem with the original patch I sent? If it needs changes or be resubmitted, I wont be able to do it within a couple of days so feel free to do it if you like. Rikard

Pierre Neidhardt

10:55 a.m.

On 15-11-02 11:49:31, Rikard Falkeborn wrote:

...

Den 2 nov 2015 08:47 skrev "Pierre Neidhardt" :

...
Since you caught the issue, Rikard, do you want to do send the patch?

-- Pierre Neidhardt

Was there a problem with the original patch I sent? If it needs changes or be resubmitted, I wont be able to do it within a couple of days so feel free to do it if you like.

Rikard

Your original patch was working. It's just that I think we can spare the 2 #defines. I'll submit the patch then. -- Pierre Neidhardt

Allan McRae

11:50 a.m.

On 02/11/15 20:55, Pierre Neidhardt wrote:

...

On 15-11-02 11:49:31, Rikard Falkeborn wrote:

...
Den 2 nov 2015 08:47 skrev "Pierre Neidhardt" :

...
Since you caught the issue, Rikard, do you want to do send the patch?

-- Pierre Neidhardt

Was there a problem with the original patch I sent? If it needs changes or be resubmitted, I wont be able to do it within a couple of days so feel free to do it if you like.

Rikard

Your original patch was working. It's just that I think we can spare the 2 #defines.

I'll submit the patch then.

Sort of... it fixed one issue but the warning given by clang still remained. Allan

Pierre Neidhardt

12:46 p.m.

On 15-11-02 21:50:20, Allan McRae wrote:

...

On 02/11/15 20:55, Pierre Neidhardt wrote:

...
On 15-11-02 11:49:31, Rikard Falkeborn wrote:

...
Den 2 nov 2015 08:47 skrev "Pierre Neidhardt" :

...
Since you caught the issue, Rikard, do you want to do send the patch?

-- Pierre Neidhardt

Was there a problem with the original patch I sent? If it needs changes or be resubmitted, I wont be able to do it within a couple of days so feel free to do it if you like.

Rikard

Your original patch was working. It's just that I think we can spare the 2 #defines.

I'll submit the patch then.

Sort of... it fixed one issue but the warning given by clang still remained.

Are you talking about the original patch? You said yourself:

...

This is actually not the cause of the warning from clang... But it does fix it (unlike all the other suggestions in the thread).

...

package.c:95:34: error: variable length array folded to constant array as an extension [-Werror,-Wgnu-folding-constant] wchar_t wbuf[ARRAYSIZE(titles)][TITLE_MAXLEN + title_suffix_len]; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ package.c:81:22: note: expanded from macro 'TITLE_MAXLEN' #define TITLE_MAXLEN 50 ^

Both the original patch and my last simplified version fix the gnu-folding-constant warning. Am I missing something? -- Pierre Neidhardt

Allan McRae

12:53 p.m.

On 02/11/15 22:46, Pierre Neidhardt wrote:

...

On 15-11-02 21:50:20, Allan McRae wrote:

...
On 02/11/15 20:55, Pierre Neidhardt wrote:

...
On 15-11-02 11:49:31, Rikard Falkeborn wrote:

...
Den 2 nov 2015 08:47 skrev "Pierre Neidhardt" :

...
Since you caught the issue, Rikard, do you want to do send the patch?

-- Pierre Neidhardt

Was there a problem with the original patch I sent? If it needs changes or be resubmitted, I wont be able to do it within a couple of days so feel free to do it if you like.

Rikard

Your original patch was working. It's just that I think we can spare the 2 #defines.

I'll submit the patch then.

Sort of... it fixed one issue but the warning given by clang still remained.

Are you talking about the original patch? You said yourself:

...
This is actually not the cause of the warning from clang... But it does fix it (unlike all the other suggestions in the thread).

...
package.c:95:34: error: variable length array folded to constant array as an extension [-Werror,-Wgnu-folding-constant] wchar_t wbuf[ARRAYSIZE(titles)][TITLE_MAXLEN + title_suffix_len]; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ package.c:81:22: note: expanded from macro 'TITLE_MAXLEN' #define TITLE_MAXLEN 50 ^

Both the original patch and my last simplified version fix the gnu-folding-constant warning. Am I missing something?

I was sure the original patch did not fix that warning in clang... oh, well. Just send in your one and I will apply. A

3109

Age (days ago)

3110

Last active (days ago)

List overview

Download

17 comments

4 participants

participants (4)

Allan McRae
Dave Reisner
Pierre Neidhardt
Rikard Falkeborn