[PATCH] Use C_ARRAY_LENGTH and C_ARRAY_END #13796
Comments
From @jhiUse C_ARRAY_LENGTH and C_ARRAY_END. Use the C_ARRAY_LENGTH instead of sizeof(c_array)/sizeof(c_array[0]) While doing this found potential off-by-one error in sv.c:Perl_sv_magic: |
From @jhi0001-Use-C_ARRAY_LENGTH-and-C_ARRAY_END.patchFrom 86696cfff6b7a222bf2475fcdb06ccfe0eafb357 Mon Sep 17 00:00:00 2001
From: Jarkko Hietaniemi <jhi@iki.fi>
Date: Fri, 2 May 2014 08:54:26 -0400
Subject: [PATCH] Use C_ARRAY_LENGTH and C_ARRAY_END.
Use the C_ARRAY_LENGTH instead of sizeof(c_array)/sizeof(c_array[0])
or sizeof(c_array)/sizeof(type_of_element_in_c_array), and C_ARRAY_END
for c_array + C_ARRAY_LENGTH(c_array).
While doing this found potential off-by-one error in sv.c:Perl_sv_magic:
how > C_ARRAY_LENGTH(PL_magic_data)
should probably have been
how >= C_ARRAY_LENGTH(PL_magic_data)
No tests fail, but this seems to be more of an internal sanity check.
---
dump.c | 5 ++---
handy.h | 7 ++++++-
patchlevel.h | 2 +-
perl.c | 2 +-
sv.c | 5 ++---
universal.c | 3 +--
util.c | 4 ++--
7 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/dump.c b/dump.c
index 354cd57..59be3e0 100644
--- a/dump.c
+++ b/dump.c
@@ -868,8 +868,7 @@ const struct op_private_by_op op_private_names[] = {
static bool
S_op_private_to_names(pTHX_ SV *tmpsv, U32 optype, U32 op_private) {
const struct op_private_by_op *start = op_private_names;
- const struct op_private_by_op *const end
- = op_private_names + C_ARRAY_LENGTH(op_private_names);
+ const struct op_private_by_op *const end = C_ARRAY_END(op_private_names);
/* This is a linear search, but no worse than the code that it replaced.
It's debugging code - size is more important than speed. */
@@ -1894,7 +1893,7 @@ Perl_do_sv_dump(pTHX_ I32 level, PerlIO *file, SV *sv, I32 nest, I32 maxnest, bo
if (HvARRAY(sv) && usedkeys) {
/* Show distribution of HEs in the ARRAY */
int freq[200];
-#define FREQ_MAX ((int)(sizeof freq / sizeof freq[0] - 1))
+#define FREQ_MAX ((int)(C_ARRAY_LENGTH(freq) - 1))
int i;
int max = 0;
U32 pow2 = 2, keys = usedkeys;
diff --git a/handy.h b/handy.h
index 3f84eff..3f2f394 100644
--- a/handy.h
+++ b/handy.h
@@ -1936,8 +1936,13 @@ void Perl_mem_log_del_sv(const SV *sv, const char *filename, const int linenumbe
#define StructCopy(s,d,t) Copy(s,d,1,t)
#endif
+/* C_ARRAY_LENGTH is the number of elements in the C array (so you
+ * want your zero-based indices to be less than but not equal to).
+ *
+ * C_ARRAY_END is one past the last: half-open/half-closed range,
+ * not last-inclusive range. */
#define C_ARRAY_LENGTH(a) (sizeof(a)/sizeof((a)[0]))
-#define C_ARRAY_END(a) (a) + (sizeof(a)/sizeof((a)[0]))
+#define C_ARRAY_END(a) ((a) + (sizeof(a)/sizeof((a)[0])))
#ifdef NEED_VA_COPY
# ifdef va_copy
diff --git a/patchlevel.h b/patchlevel.h
index 6567b2f..acd8925 100644
--- a/patchlevel.h
+++ b/patchlevel.h
@@ -144,7 +144,7 @@ static const char * const local_patches[] = {
/* Initial space prevents this variable from being inserted in config.sh */
# define LOCAL_PATCH_COUNT \
- ((int)(sizeof(local_patches)/sizeof(local_patches[0])-2))
+ ((int)(C_ARRAY_LENGTH(local_patches)-2))
/* the old terms of reference, add them only when explicitly included */
#define PATCHLEVEL PERL_VERSION
diff --git a/perl.c b/perl.c
index f5007f7..023748c 100644
--- a/perl.c
+++ b/perl.c
@@ -677,7 +677,7 @@ perl_destruct(pTHXx)
msg.msg_name = NULL;
msg.msg_namelen = 0;
msg.msg_iov = vec;
- msg.msg_iovlen = sizeof(vec)/sizeof(vec[0]);
+ msg.msg_iovlen = C_ARRAY_LENGTH(vec);
vec[0].iov_base = (void*)⌖
vec[0].iov_len = sizeof(target);
diff --git a/sv.c b/sv.c
index 85f91f1..971bf21 100644
--- a/sv.c
+++ b/sv.c
@@ -5557,7 +5557,7 @@ Perl_sv_magic(pTHX_ SV *const sv, SV *const obj, const int how,
PERL_ARGS_ASSERT_SV_MAGIC;
- if (how < 0 || (unsigned)how > C_ARRAY_LENGTH(PL_magic_data)
+ if (how < 0 || (unsigned)how >= C_ARRAY_LENGTH(PL_magic_data)
|| ((flags = PL_magic_data[how]),
(vtable_index = flags & PERL_MAGIC_VTABLE_MASK)
> magic_vtable_max))
@@ -12125,8 +12125,7 @@ Perl_ptr_table_store(pTHX_ PTR_TBL_t *const tbl, const void *const oldsv, void *
new_arena->next = tbl->tbl_arena;
tbl->tbl_arena = new_arena;
tbl->tbl_arena_next = new_arena->array;
- tbl->tbl_arena_end = new_arena->array
- + sizeof(new_arena->array) / sizeof(new_arena->array[0]);
+ tbl->tbl_arena_end = C_ARRAY_END(new_arena->array);
}
tblent = tbl->tbl_arena_next++;
diff --git a/universal.c b/universal.c
index bccc8fb..a29696d 100644
--- a/universal.c
+++ b/universal.c
@@ -1059,8 +1059,7 @@ Perl_boot_core_UNIVERSAL(pTHX)
dVAR;
static const char file[] = __FILE__;
const struct xsub_details *xsub = details;
- const struct xsub_details *end
- = details + sizeof(details) / sizeof(details[0]);
+ const struct xsub_details *end = C_ARRAY_END(details);
do {
newXS_flags(xsub->name, xsub->xsub, file, xsub->proto, 0);
diff --git a/util.c b/util.c
index 0a0ee40..b90abe5 100644
--- a/util.c
+++ b/util.c
@@ -4576,8 +4576,8 @@ Perl_init_global_struct(pTHX)
{
struct perl_vars *plvarsp = NULL;
# ifdef PERL_GLOBAL_STRUCT
- const IV nppaddr = sizeof(Gppaddr)/sizeof(Perl_ppaddr_t);
- const IV ncheck = sizeof(Gcheck) /sizeof(Perl_check_t);
+ const IV nppaddr = C_ARRAY_LENGTH(Gppaddr);
+ const IV ncheck = C_ARRAY_LENGTH(Gcheck);
# ifdef PERL_GLOBAL_STRUCT_PRIVATE
/* PerlMem_malloc() because can't use even safesysmalloc() this early. */
plvarsp = (struct perl_vars*)PerlMem_malloc(sizeof(struct perl_vars));
--
1.9.2
|
From @shlomifOn Fri May 02 06:02:20 2014, jhi wrote:
Looks like a useful patch. Thanks! |
|
The RT System itself - Status changed from 'new' to 'open' |
From @tonycozOn Fri May 02 06:02:20 2014, jhi wrote:
Looks good, added to 5.21.1 blockers. Tony |
From @bulk88Is this Coverity or hand found issue? Just curious. -- |
From @jhiOn Saturday-201405-10, 17:20, bulk88 via RT wrote:
Not Coverity-found as such, but "inspired", since first Coverity had And then I decided to inspect the code for similar spots, since I knew And while I did that, I noticed that one spot where use of this macro |
From @jhiOn Monday-201405-05, 20:41, Tony Cook via RT wrote:
Updated patch: found one more spot in regcomp.c, and now using |
From @jhi0001-Use-C_ARRAY_LENGTH-and-C_ARRAY_END.patchFrom b2f369a8bd74859d06a9315ffa208e3fbebb2099 Mon Sep 17 00:00:00 2001
From: Jarkko Hietaniemi <jhi@iki.fi>
Date: Fri, 2 May 2014 08:54:26 -0400
Subject: [PATCH] Use C_ARRAY_LENGTH and C_ARRAY_END.
Use the C_ARRAY_LENGTH instead of sizeof(c_array)/sizeof(c_array[0])
or sizeof(c_array)/sizeof(type_of_element_in_c_array), and C_ARRAY_END
for c_array + C_ARRAY_LENGTH(c_array).
While doing this found
(1) definite off-by-one error in regcomp.c:Perl_regprop
if (index > (sizeof(anyofs) / sizeof(anyofs[0]))) {
(for failure) needed to be
if (index < C_ARRAY_LENGTH(anyofs)) {
(2) potential off-by-one error in sv.c:Perl_sv_magic:
how > C_ARRAY_LENGTH(PL_magic_data)
(for failure) should probably have been
how >= C_ARRAY_LENGTH(PL_magic_data)
No tests fail, but this seems to be more of an internal sanity check.
---
dump.c | 5 ++---
handy.h | 7 ++++++-
patchlevel.h | 2 +-
perl.c | 2 +-
regcomp.c | 8 ++++----
sv.c | 5 ++---
universal.c | 3 +--
util.c | 4 ++--
8 files changed, 19 insertions(+), 17 deletions(-)
diff --git a/dump.c b/dump.c
index 354cd57..59be3e0 100644
--- a/dump.c
+++ b/dump.c
@@ -868,8 +868,7 @@ const struct op_private_by_op op_private_names[] = {
static bool
S_op_private_to_names(pTHX_ SV *tmpsv, U32 optype, U32 op_private) {
const struct op_private_by_op *start = op_private_names;
- const struct op_private_by_op *const end
- = op_private_names + C_ARRAY_LENGTH(op_private_names);
+ const struct op_private_by_op *const end = C_ARRAY_END(op_private_names);
/* This is a linear search, but no worse than the code that it replaced.
It's debugging code - size is more important than speed. */
@@ -1894,7 +1893,7 @@ Perl_do_sv_dump(pTHX_ I32 level, PerlIO *file, SV *sv, I32 nest, I32 maxnest, bo
if (HvARRAY(sv) && usedkeys) {
/* Show distribution of HEs in the ARRAY */
int freq[200];
-#define FREQ_MAX ((int)(sizeof freq / sizeof freq[0] - 1))
+#define FREQ_MAX ((int)(C_ARRAY_LENGTH(freq) - 1))
int i;
int max = 0;
U32 pow2 = 2, keys = usedkeys;
diff --git a/handy.h b/handy.h
index 3f84eff..9332f8d 100644
--- a/handy.h
+++ b/handy.h
@@ -1936,8 +1936,13 @@ void Perl_mem_log_del_sv(const SV *sv, const char *filename, const int linenumbe
#define StructCopy(s,d,t) Copy(s,d,1,t)
#endif
+/* C_ARRAY_LENGTH is the number of elements in the C array (so you
+ * want your zero-based indices to be less than but not equal to).
+ *
+ * C_ARRAY_END is one past the last: half-open/half-closed range,
+ * not last-inclusive range. */
#define C_ARRAY_LENGTH(a) (sizeof(a)/sizeof((a)[0]))
-#define C_ARRAY_END(a) (a) + (sizeof(a)/sizeof((a)[0]))
+#define C_ARRAY_END(a) ((a) + C_ARRAY_LENGTH(a))
#ifdef NEED_VA_COPY
# ifdef va_copy
diff --git a/patchlevel.h b/patchlevel.h
index 6567b2f..acd8925 100644
--- a/patchlevel.h
+++ b/patchlevel.h
@@ -144,7 +144,7 @@ static const char * const local_patches[] = {
/* Initial space prevents this variable from being inserted in config.sh */
# define LOCAL_PATCH_COUNT \
- ((int)(sizeof(local_patches)/sizeof(local_patches[0])-2))
+ ((int)(C_ARRAY_LENGTH(local_patches)-2))
/* the old terms of reference, add them only when explicitly included */
#define PATCHLEVEL PERL_VERSION
diff --git a/perl.c b/perl.c
index 27d0d9e..0fb87dc 100644
--- a/perl.c
+++ b/perl.c
@@ -677,7 +677,7 @@ perl_destruct(pTHXx)
msg.msg_name = NULL;
msg.msg_namelen = 0;
msg.msg_iov = vec;
- msg.msg_iovlen = sizeof(vec)/sizeof(vec[0]);
+ msg.msg_iovlen = C_ARRAY_LENGTH(vec);
vec[0].iov_base = (void*)⌖
vec[0].iov_len = sizeof(target);
diff --git a/regcomp.c b/regcomp.c
index ca2ffb8..0238af9 100644
--- a/regcomp.c
+++ b/regcomp.c
@@ -15831,10 +15831,7 @@ Perl_regprop(pTHX_ const regexp *prog, SV *sv, const regnode *o, const regmatch_
}
else if (k == POSIXD || k == NPOSIXD) {
U8 index = FLAGS(o) * 2;
- if (index > (sizeof(anyofs) / sizeof(anyofs[0]))) {
- Perl_sv_catpvf(aTHX_ sv, "[illegal type=%d])", index);
- }
- else {
+ if (index < C_ARRAY_LENGTH(anyofs)) {
if (*anyofs[index] != '[') {
sv_catpv(sv, "[");
}
@@ -15843,6 +15840,9 @@ Perl_regprop(pTHX_ const regexp *prog, SV *sv, const regnode *o, const regmatch_
sv_catpv(sv, "]");
}
}
+ else {
+ Perl_sv_catpvf(aTHX_ sv, "[illegal type=%d])", index);
+ }
}
else if (k == BRANCHJ && (OP(o) == UNLESSM || OP(o) == IFMATCH))
Perl_sv_catpvf(aTHX_ sv, "[%d]", -(o->flags));
diff --git a/sv.c b/sv.c
index 85f91f1..971bf21 100644
--- a/sv.c
+++ b/sv.c
@@ -5557,7 +5557,7 @@ Perl_sv_magic(pTHX_ SV *const sv, SV *const obj, const int how,
PERL_ARGS_ASSERT_SV_MAGIC;
- if (how < 0 || (unsigned)how > C_ARRAY_LENGTH(PL_magic_data)
+ if (how < 0 || (unsigned)how >= C_ARRAY_LENGTH(PL_magic_data)
|| ((flags = PL_magic_data[how]),
(vtable_index = flags & PERL_MAGIC_VTABLE_MASK)
> magic_vtable_max))
@@ -12125,8 +12125,7 @@ Perl_ptr_table_store(pTHX_ PTR_TBL_t *const tbl, const void *const oldsv, void *
new_arena->next = tbl->tbl_arena;
tbl->tbl_arena = new_arena;
tbl->tbl_arena_next = new_arena->array;
- tbl->tbl_arena_end = new_arena->array
- + sizeof(new_arena->array) / sizeof(new_arena->array[0]);
+ tbl->tbl_arena_end = C_ARRAY_END(new_arena->array);
}
tblent = tbl->tbl_arena_next++;
diff --git a/universal.c b/universal.c
index bccc8fb..a29696d 100644
--- a/universal.c
+++ b/universal.c
@@ -1059,8 +1059,7 @@ Perl_boot_core_UNIVERSAL(pTHX)
dVAR;
static const char file[] = __FILE__;
const struct xsub_details *xsub = details;
- const struct xsub_details *end
- = details + sizeof(details) / sizeof(details[0]);
+ const struct xsub_details *end = C_ARRAY_END(details);
do {
newXS_flags(xsub->name, xsub->xsub, file, xsub->proto, 0);
diff --git a/util.c b/util.c
index 0a0ee40..b90abe5 100644
--- a/util.c
+++ b/util.c
@@ -4576,8 +4576,8 @@ Perl_init_global_struct(pTHX)
{
struct perl_vars *plvarsp = NULL;
# ifdef PERL_GLOBAL_STRUCT
- const IV nppaddr = sizeof(Gppaddr)/sizeof(Perl_ppaddr_t);
- const IV ncheck = sizeof(Gcheck) /sizeof(Perl_check_t);
+ const IV nppaddr = C_ARRAY_LENGTH(Gppaddr);
+ const IV ncheck = C_ARRAY_LENGTH(Gcheck);
# ifdef PERL_GLOBAL_STRUCT_PRIVATE
/* PerlMem_malloc() because can't use even safesysmalloc() this early. */
plvarsp = (struct perl_vars*)PerlMem_malloc(sizeof(struct perl_vars));
--
1.9.2
|
From @tonycozOn Mon May 12 08:29:42 2014, jhi wrote:
Conflicts due to 53673d9. Tony |
From @jhiOn Monday-201405-19, 3:00, Tony Cook via RT wrote:
Updated patch attached. |
From @jhi0001-Use-the-C_ARRAY_LENGTH.patchFrom 2c2ee2c3260e8b11176ea98e868583964b1517df Mon Sep 17 00:00:00 2001
From: Jarkko Hietaniemi <jhi@iki.fi>
Date: Mon, 19 May 2014 06:52:24 -0400
Subject: [PATCH] Use the C_ARRAY_LENGTH.
Use the C_ARRAY_LENGTH instead of sizeof(c_array)/sizeof(c_array[0])
or sizeof(c_array)/sizeof(type_of_element_in_c_array), and C_ARRAY_END
for c_array + C_ARRAY_LENGTH(c_array).
While doing this found potential off-by-one error in sv.c:Perl_sv_magic:
how > C_ARRAY_LENGTH(PL_magic_data)
should probably have been
how >= C_ARRAY_LENGTH(PL_magic_data)
No tests fail, but this seems to be more of an internal sanity check.
---
dump.c | 5 ++---
handy.h | 7 ++++++-
patchlevel.h | 2 +-
perl.c | 2 +-
sv.c | 5 ++---
universal.c | 3 +--
util.c | 4 ++--
7 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/dump.c b/dump.c
index 354cd57..59be3e0 100644
--- a/dump.c
+++ b/dump.c
@@ -868,8 +868,7 @@ const struct op_private_by_op op_private_names[] = {
static bool
S_op_private_to_names(pTHX_ SV *tmpsv, U32 optype, U32 op_private) {
const struct op_private_by_op *start = op_private_names;
- const struct op_private_by_op *const end
- = op_private_names + C_ARRAY_LENGTH(op_private_names);
+ const struct op_private_by_op *const end = C_ARRAY_END(op_private_names);
/* This is a linear search, but no worse than the code that it replaced.
It's debugging code - size is more important than speed. */
@@ -1894,7 +1893,7 @@ Perl_do_sv_dump(pTHX_ I32 level, PerlIO *file, SV *sv, I32 nest, I32 maxnest, bo
if (HvARRAY(sv) && usedkeys) {
/* Show distribution of HEs in the ARRAY */
int freq[200];
-#define FREQ_MAX ((int)(sizeof freq / sizeof freq[0] - 1))
+#define FREQ_MAX ((int)(C_ARRAY_LENGTH(freq) - 1))
int i;
int max = 0;
U32 pow2 = 2, keys = usedkeys;
diff --git a/handy.h b/handy.h
index 3f84eff..9332f8d 100644
--- a/handy.h
+++ b/handy.h
@@ -1936,8 +1936,13 @@ void Perl_mem_log_del_sv(const SV *sv, const char *filename, const int linenumbe
#define StructCopy(s,d,t) Copy(s,d,1,t)
#endif
+/* C_ARRAY_LENGTH is the number of elements in the C array (so you
+ * want your zero-based indices to be less than but not equal to).
+ *
+ * C_ARRAY_END is one past the last: half-open/half-closed range,
+ * not last-inclusive range. */
#define C_ARRAY_LENGTH(a) (sizeof(a)/sizeof((a)[0]))
-#define C_ARRAY_END(a) (a) + (sizeof(a)/sizeof((a)[0]))
+#define C_ARRAY_END(a) ((a) + C_ARRAY_LENGTH(a))
#ifdef NEED_VA_COPY
# ifdef va_copy
diff --git a/patchlevel.h b/patchlevel.h
index 0b50d75..b045f12 100644
--- a/patchlevel.h
+++ b/patchlevel.h
@@ -145,7 +145,7 @@ static const char * const local_patches[] = {
/* Initial space prevents this variable from being inserted in config.sh */
# define LOCAL_PATCH_COUNT \
- ((int)(sizeof(local_patches)/sizeof(local_patches[0])-2))
+ ((int)(C_ARRAY_LENGTH(local_patches)-2))
/* the old terms of reference, add them only when explicitly included */
#define PATCHLEVEL PERL_VERSION
diff --git a/perl.c b/perl.c
index 27d0d9e..0fb87dc 100644
--- a/perl.c
+++ b/perl.c
@@ -677,7 +677,7 @@ perl_destruct(pTHXx)
msg.msg_name = NULL;
msg.msg_namelen = 0;
msg.msg_iov = vec;
- msg.msg_iovlen = sizeof(vec)/sizeof(vec[0]);
+ msg.msg_iovlen = C_ARRAY_LENGTH(vec);
vec[0].iov_base = (void*)⌖
vec[0].iov_len = sizeof(target);
diff --git a/sv.c b/sv.c
index 1005313..07992a3 100644
--- a/sv.c
+++ b/sv.c
@@ -5581,7 +5581,7 @@ Perl_sv_magic(pTHX_ SV *const sv, SV *const obj, const int how,
PERL_ARGS_ASSERT_SV_MAGIC;
- if (how < 0 || (unsigned)how > C_ARRAY_LENGTH(PL_magic_data)
+ if (how < 0 || (unsigned)how >= C_ARRAY_LENGTH(PL_magic_data)
|| ((flags = PL_magic_data[how]),
(vtable_index = flags & PERL_MAGIC_VTABLE_MASK)
> magic_vtable_max))
@@ -12257,8 +12257,7 @@ Perl_ptr_table_store(pTHX_ PTR_TBL_t *const tbl, const void *const oldsv, void *
new_arena->next = tbl->tbl_arena;
tbl->tbl_arena = new_arena;
tbl->tbl_arena_next = new_arena->array;
- tbl->tbl_arena_end = new_arena->array
- + sizeof(new_arena->array) / sizeof(new_arena->array[0]);
+ tbl->tbl_arena_end = C_ARRAY_END(new_arena->array);
}
tblent = tbl->tbl_arena_next++;
diff --git a/universal.c b/universal.c
index bccc8fb..a29696d 100644
--- a/universal.c
+++ b/universal.c
@@ -1059,8 +1059,7 @@ Perl_boot_core_UNIVERSAL(pTHX)
dVAR;
static const char file[] = __FILE__;
const struct xsub_details *xsub = details;
- const struct xsub_details *end
- = details + sizeof(details) / sizeof(details[0]);
+ const struct xsub_details *end = C_ARRAY_END(details);
do {
newXS_flags(xsub->name, xsub->xsub, file, xsub->proto, 0);
diff --git a/util.c b/util.c
index 0a0ee40..b90abe5 100644
--- a/util.c
+++ b/util.c
@@ -4576,8 +4576,8 @@ Perl_init_global_struct(pTHX)
{
struct perl_vars *plvarsp = NULL;
# ifdef PERL_GLOBAL_STRUCT
- const IV nppaddr = sizeof(Gppaddr)/sizeof(Perl_ppaddr_t);
- const IV ncheck = sizeof(Gcheck) /sizeof(Perl_check_t);
+ const IV nppaddr = C_ARRAY_LENGTH(Gppaddr);
+ const IV ncheck = C_ARRAY_LENGTH(Gcheck);
# ifdef PERL_GLOBAL_STRUCT_PRIVATE
/* PerlMem_malloc() because can't use even safesysmalloc() this early. */
plvarsp = (struct perl_vars*)PerlMem_malloc(sizeof(struct perl_vars));
--
1.8.5.2 (Apple Git-48)
|
|
@tsee - Status changed from 'open' to 'resolved' |
Migrated from rt.perl.org#121786 (status was 'resolved')
Searchable as RT121786$
The text was updated successfully, but these errors were encountered: