[pacman-dev] [PATCH] package.c, fix incorrect buffersize
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 <rikard.falkeborn@gmail.com> --- 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
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
Unless you want to do it of course :) -- Pierre Neidhardt Knowledge without common sense is folly.
Den 1 nov 2015 09:36 skrev "Pierre Neidhardt" <ambrevar@gmail.com>:
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
Den 1 nov 2015 09:55 skrev "Rikard Falkeborn" <rikard.falkeborn@gmail.com>:
Den 1 nov 2015 09:36 skrev "Pierre Neidhardt" <ambrevar@gmail.com>:
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
On 15-11-01 10:05:30, Rikard Falkeborn wrote:
Den 1 nov 2015 09:55 skrev "Rikard Falkeborn" <rikard.falkeborn@gmail.com>:
Den 1 nov 2015 09:36 skrev "Pierre Neidhardt" <ambrevar@gmail.com>:
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
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"
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 <rikard.falkeborn@gmail.com> --- 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])); } }
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
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 <rikard.falkeborn@gmail.com> ---
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
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 <rikard.falkeborn@gmail.com> ---
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
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 <rikard.falkeborn@gmail.com> ---
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
Since you caught the issue, Rikard, do you want to do send the patch? -- Pierre Neidhardt
Den 2 nov 2015 08:47 skrev "Pierre Neidhardt" <ambrevar@gmail.com>:
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
On 15-11-02 11:49:31, Rikard Falkeborn wrote:
Den 2 nov 2015 08:47 skrev "Pierre Neidhardt" <ambrevar@gmail.com>:
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
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" <ambrevar@gmail.com>:
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
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" <ambrevar@gmail.com>:
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
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" <ambrevar@gmail.com>:
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
participants (4)
-
Allan McRae
-
Dave Reisner
-
Pierre Neidhardt
-
Rikard Falkeborn