Re: malloc in libssl/src/apps
On Sun, May 04, 2014 at 02:38:40AM -0400, Jean-Philippe Ouellet wrote:
Hello,
I've gone through lib/libssl/src/apps with the goal of making {m,c,re}alloc
uses more idiomatic, adding error checking in some places where missing,
and some minor style unification.
Feedback appreciated, better patches to come after the semester ends.
Index: apps.c
===
RCS file: /cvs/src/lib/libssl/src/apps/apps.c,v
retrieving revision 1.45
diff -u -p -r1.45 apps.c
--- apps.c3 May 2014 16:03:54 - 1.45
+++ apps.c4 May 2014 06:07:45 -
@@ -209,13 +209,12 @@ chopup_args(ARGS * arg, char *buf, int *
*argc = 0;
*argv = NULL;
- i = 0;
if (arg-count == 0) {
arg-count = 20;
- arg-data = (char **)malloc(sizeof(char *) * arg-count);
+ arg-data = calloc(arg-count, sizeof(char *));
+ if (arg-data == NULL)
+ return 0;
}
- for (i = 0; i arg-count; i++)
- arg-data[i] = NULL;
num = 0;
p = buf;
@@ -232,8 +231,7 @@ chopup_args(ARGS * arg, char *buf, int *
if (num = arg-count) {
char **tmp_p;
int tlen = arg-count + 20;
- tmp_p = (char **) realloc(arg-data,
- sizeof(char *) * tlen);
+ tmp_p = reallocarray(arg-data, tlen, sizeof(char *));
if (tmp_p == NULL)
return 0;
arg-data = tmp_p;
@@ -266,7 +264,7 @@ chopup_args(ARGS * arg, char *buf, int *
}
*argc = num;
*argv = arg-data;
- return (1);
+ return 1;
}
int
@@ -404,7 +402,28 @@ password_callback(char *buf, int bufsiz,
ok = UI_add_input_string(ui, prompt, ui_flags, buf,
PW_MIN_LENGTH, bufsiz - 1);
if (ok = 0 verify) {
- buff = (char *) malloc(bufsiz);
+ buff = malloc(bufsiz);
+ /*
+ * NULL returns appear to be handled by the following:
+ *
+ * UI_add_verify_string(x, x, x, buff=NULL, x, x, x) -
+ * general_allocate_string(x, x, x, UIT_VERIFY, x, \
+ * result_buf=NULL, x, x, x) -
+ * general_allocate_prompt(x, x, x, x, x, NULL) -
+ * ret = NULL;
+ * if (type == UIT_VERIFY result_buf == NULL)
+ * UIerr(...); and dont touch ret
+ * returns NULL
+ * returns -1
+ * returns -1
+ *
+ * So, we /should/ (maybe) be good. Not calling UIerr()
+ * could very well have some well-hidden side-effects
+ * that I would definitly not notice myself, so I'll
+ * leave this as is without an explicit check here.
+ * Maybe somebody who knows better than I has a better
+ * suggestion?
+ */
ok = UI_add_verify_string(ui, prompt, ui_flags, buff,
PW_MIN_LENGTH, bufsiz - 1, buf);
}
@@ -1830,26 +1849,30 @@ parse_yesno(const char *str, int def)
X509_NAME *
parse_name(char *subject, long chtype, int multirdn)
{
- size_t buflen = strlen(subject) + 1;/* to copy the types and
- * values into. due to
- * escaping, the copy can
- * only become shorter */
- char *buf = malloc(buflen);
- size_t max_ne = buflen / 2 + 1; /* maximum number of name elements */
- char **ne_types = malloc(max_ne * sizeof(char *));
- char **ne_values = malloc(max_ne * sizeof(char *));
- int *mval = malloc(max_ne * sizeof(int));
+ size_t buflen, max_ne;
+ char **ne_types, **ne_values, *buf, *sp, *bp;
+ int *mval, i, nid, ne_num = 0;
+ X509_NAME *n = NULL;
- char *sp = subject, *bp = buf;
- int i, ne_num = 0;
+ /* Due to escaping, buf can never be bigger than subject. */
+ buflen = strlen(subject + 1);
Fail!
- X509_NAME *n = NULL;
- int nid;
+ /* maximum number of name elements */
+ max_ne = buflen / 2 + 1;
+
+ buf = malloc(buflen);
+ ne_types = malloc(max_ne);
+ ne_values = malloc(max_ne);
+ mval = reallocarray(NULL, max_ne, sizeof(int));
why not use calloc(2)?
if (!buf || !ne_types || !ne_values || !mval) {
BIO_printf(bio_err, malloc error\n);
goto error;
}
+
+ sp
Re: malloc in libssl/src/apps
On Sun, May 4, 2014 at 12:21 AM, patrick keshishian sids...@boxsoft.comwrote:
On Sun, May 04, 2014 at 02:38:40AM -0400, Jean-Philippe Ouellet wrote:
...
- if ((irow = (char **)malloc(sizeof(char *) *
- (DB_NUMBER + 1))) == NULL) {
+ irow = reallocarray(NULL, DB_NUMBER + 1, sizeof(char *));
why not use calloc(2)?
Please justify your belief that the existing code is wrong by lack of
memset/bzero. Please do so for *ALL* the places where you suggested using
calloc() where Jean-Philippe Ouellet's patch suggest reallocarray().
Either:
a) the existing code's lack of memset/bzero is a bug (details?), OR
b) using calloc() instead of reallocarray() is *wrong*.
Philip Guenther
Re: malloc in libssl/src/apps
On Sun, May 04, 2014 at 12:29:59AM -0700, Philip Guenther wrote:
On Sun, May 4, 2014 at 12:21 AM, patrick keshishian
sids...@boxsoft.comwrote:
On Sun, May 04, 2014 at 02:38:40AM -0400, Jean-Philippe Ouellet wrote:
...
- if ((irow = (char **)malloc(sizeof(char *) *
- (DB_NUMBER + 1))) == NULL) {
+ irow = reallocarray(NULL, DB_NUMBER + 1, sizeof(char *));
why not use calloc(2)?
Please justify your belief that the existing code is wrong by lack of
memset/bzero. Please do so for *ALL* the places where you suggested using
calloc() where Jean-Philippe Ouellet's patch suggest reallocarray().
How did you deduce my belief to be such?
The code was changing malloc(n * size) to reallocarray().
Typical correction for this usage has been to convert to
calloc(2), in order to avoid possible integer overflows.
Seeing that reallocarray() appeared in OpenBSD 5.6, I am
sure it is an equally fine interface.
Any concern over the first strlen() change I pointed out?
--patrick
Either:
a) the existing code's lack of memset/bzero is a bug (details?), OR
b) using calloc() instead of reallocarray() is *wrong*.
Philip Guenther
Re: malloc in libssl/src/apps
On Sun, May 04, 2014 at 12:21:04AM -0700, patrick keshishian wrote: why not use calloc(2)? Because it doesn't exist ? (hint: the 2 in calloc(2) is the manual section. There is no calloc system call, therefore you can't be right. See guenther(2) for a more serious answer).
Re: malloc in libssl/src/apps
On Sunday, May 4, 2014, patrick keshishian sids...@boxsoft.com wrote:
On Sun, May 04, 2014 at 12:29:59AM -0700, Philip Guenther wrote:
On Sun, May 4, 2014 at 12:21 AM, patrick keshishian
sids...@boxsoft.comjavascript:;
wrote:
On Sun, May 04, 2014 at 02:38:40AM -0400, Jean-Philippe Ouellet wrote:
...
- if ((irow = (char **)malloc(sizeof(char *) *
- (DB_NUMBER + 1))) == NULL) {
+ irow = reallocarray(NULL, DB_NUMBER + 1, sizeof(char
*));
why not use calloc(2)?
Please justify your belief that the existing code is wrong by lack of
memset/bzero. Please do so for *ALL* the places where you suggested
using
calloc() where Jean-Philippe Ouellet's patch suggest reallocarray().
How did you deduce my belief to be such?
The difference between reallocarray(NULL, x, y) and calloc(x, y) is that
the latter zeros the allocated array. Ergo, your suggestion to use calloc()
instead of reallocarray() indicates a belief that the array should be
zeroed, meaning the existing code is wrong.
The code was changing malloc(n * size) to reallocarray().
Typical correction for this usage has been to convert to
calloc(2), in order to avoid possible integer overflows.
Seeing that reallocarray() appeared in OpenBSD 5.6, I am
sure it is an equally fine interface.
Please *support* the use of reallocarray() in code that would otherwise
have to do the overflow check itself (and possibly screw it up) or use
calloc() (and zero possibly large chunks of memory unnecessarily)!
Philip Guenther
Re: malloc in libssl/src/apps
On Sun, May 04, 2014 at 01:26:18AM -0700, Philip Guenther wrote:
On Sunday, May 4, 2014, patrick keshishian sids...@boxsoft.com wrote:
On Sun, May 04, 2014 at 12:29:59AM -0700, Philip Guenther wrote:
On Sun, May 4, 2014 at 12:21 AM, patrick keshishian
sids...@boxsoft.comjavascript:;
wrote:
On Sun, May 04, 2014 at 02:38:40AM -0400, Jean-Philippe Ouellet wrote:
...
- if ((irow = (char **)malloc(sizeof(char *) *
- (DB_NUMBER + 1))) == NULL) {
+ irow = reallocarray(NULL, DB_NUMBER + 1, sizeof(char
*));
why not use calloc(2)?
Please justify your belief that the existing code is wrong by lack of
memset/bzero. Please do so for *ALL* the places where you suggested
using
calloc() where Jean-Philippe Ouellet's patch suggest reallocarray().
How did you deduce my belief to be such?
The difference between reallocarray(NULL, x, y) and calloc(x, y) is that
the latter zeros the allocated array. Ergo, your suggestion to use calloc()
instead of reallocarray() indicates a belief that the array should be
zeroed, meaning the existing code is wrong.
The code was changing malloc(n * size) to reallocarray().
Typical correction for this usage has been to convert to
calloc(2), in order to avoid possible integer overflows.
Seeing that reallocarray() appeared in OpenBSD 5.6, I am
sure it is an equally fine interface.
Please *support* the use of reallocarray() in code that would otherwise
have to do the overflow check itself (and possibly screw it up) or use
calloc() (and zero possibly large chunks of memory unnecessarily)!
That sentence does not make sense. I have read it multiple
times.
I have no idea what you are driving at, other than diverting
attention away from the strlen() mistake (possibly typo) in
the original suggested patch.
That's the real issue any one serious should care about.
Unless, of course, you want to tell me the following two
statements are identical:
1. strlen(subject) + 1;/* original code */
2. strlen(subject + 1);/* suggested patch */
--patrick
sftp zap extra whitespace
Hi All, An extra whitespace can be removed here: Index: sftp.c === RCS file: /cvs/src/usr.bin/ssh/sftp.c,v retrieving revision 1.162 diff -u -p -u -p -r1.162 sftp.c --- sftp.c 29 Apr 2014 20:36:51 - 1.162 +++ sftp.c 4 May 2014 09:26:56 - @@ -707,7 +707,7 @@ process_put(struct sftp_conn *conn, char resume |= global_aflag; if (!quiet resume) - printf(Resuming upload of %s to %s\n, g.gl_pathv[i], + printf(Resuming upload of %s to %s\n, g.gl_pathv[i], abs_dst); else if (!quiet !resume) printf(Uploading %s to %s\n, g.gl_pathv[i], abs_dst);
ssh regression suite connect-privsep.sh issue
Hi All,
The 'Z' flag was removed 10 days ago by Ted.
connect-privsep.sh complains that there is an unknown malloc option.
Diff below:
Index: connect-privsep.sh
===
RCS file: /cvs/src/regress/usr.bin/ssh/connect-privsep.sh,v
retrieving revision 1.4
diff -u -p -u -p -r1.4 connect-privsep.sh
--- connect-privsep.sh 2 Jul 2012 14:37:06 - 1.4
+++ connect-privsep.sh 4 May 2014 10:15:50 -
@@ -25,7 +25,7 @@ done
# Because sandbox is sensitive to changes in libc, especially malloc, retest
# with every malloc.conf option (and none).
-for m in '' A F G H J P R S X Z '' ''; do
+for m in '' A F G H J P R S X '' ''; do
for p in 1 2; do
env MALLOC_OPTIONS=$m ${SSH} -$p -F $OBJ/ssh_proxy 999.999.999.999
true
if [ $? -ne 0 ]; then
Re: malloc in libssl/src/apps
On Sun, May 04, 2014 at 02:38:40AM -0400, Jean-Philippe Ouellet wrote:
Hello,
I've gone through lib/libssl/src/apps with the goal of making {m,c,re}alloc
uses more idiomatic, adding error checking in some places where missing,
and some minor style unification.
Feedback appreciated, better patches to come after the semester ends.
Index: apps.c
===
RCS file: /cvs/src/lib/libssl/src/apps/apps.c,v
retrieving revision 1.45
diff -u -p -r1.45 apps.c
--- apps.c3 May 2014 16:03:54 - 1.45
+++ apps.c4 May 2014 06:07:45 -
@@ -209,13 +209,12 @@ chopup_args(ARGS * arg, char *buf, int *
*argc = 0;
*argv = NULL;
- i = 0;
if (arg-count == 0) {
arg-count = 20;
- arg-data = (char **)malloc(sizeof(char *) * arg-count);
+ arg-data = calloc(arg-count, sizeof(char *));
+ if (arg-data == NULL)
+ return 0;
}
- for (i = 0; i arg-count; i++)
- arg-data[i] = NULL;
num = 0;
p = buf;
@@ -232,8 +231,7 @@ chopup_args(ARGS * arg, char *buf, int *
if (num = arg-count) {
char **tmp_p;
int tlen = arg-count + 20;
- tmp_p = (char **) realloc(arg-data,
- sizeof(char *) * tlen);
+ tmp_p = reallocarray(arg-data, tlen, sizeof(char *));
if (tmp_p == NULL)
return 0;
arg-data = tmp_p;
@@ -266,7 +264,7 @@ chopup_args(ARGS * arg, char *buf, int *
}
*argc = num;
*argv = arg-data;
- return (1);
+ return 1;
}
int
@@ -404,7 +402,28 @@ password_callback(char *buf, int bufsiz,
ok = UI_add_input_string(ui, prompt, ui_flags, buf,
PW_MIN_LENGTH, bufsiz - 1);
if (ok = 0 verify) {
- buff = (char *) malloc(bufsiz);
+ buff = malloc(bufsiz);
+ /*
+ * NULL returns appear to be handled by the following:
+ *
+ * UI_add_verify_string(x, x, x, buff=NULL, x, x, x) -
+ * general_allocate_string(x, x, x, UIT_VERIFY, x, \
+ * result_buf=NULL, x, x, x) -
+ * general_allocate_prompt(x, x, x, x, x, NULL) -
+ * ret = NULL;
+ * if (type == UIT_VERIFY result_buf == NULL)
+ * UIerr(...); and dont touch ret
+ * returns NULL
+ * returns -1
+ * returns -1
+ *
+ * So, we /should/ (maybe) be good. Not calling UIerr()
+ * could very well have some well-hidden side-effects
+ * that I would definitly not notice myself, so I'll
+ * leave this as is without an explicit check here.
+ * Maybe somebody who knows better than I has a better
+ * suggestion?
+ */
ok = UI_add_verify_string(ui, prompt, ui_flags, buff,
PW_MIN_LENGTH, bufsiz - 1, buf);
}
@@ -1830,26 +1849,30 @@ parse_yesno(const char *str, int def)
X509_NAME *
parse_name(char *subject, long chtype, int multirdn)
{
- size_t buflen = strlen(subject) + 1;/* to copy the types and
- * values into. due to
- * escaping, the copy can
- * only become shorter */
- char *buf = malloc(buflen);
- size_t max_ne = buflen / 2 + 1; /* maximum number of name elements */
- char **ne_types = malloc(max_ne * sizeof(char *));
- char **ne_values = malloc(max_ne * sizeof(char *));
- int *mval = malloc(max_ne * sizeof(int));
+ size_t buflen, max_ne;
+ char **ne_types, **ne_values, *buf, *sp, *bp;
+ int *mval, i, nid, ne_num = 0;
+ X509_NAME *n = NULL;
- char *sp = subject, *bp = buf;
- int i, ne_num = 0;
+ /* Due to escaping, buf can never be bigger than subject. */
+ buflen = strlen(subject + 1);
In addition to already mentioned mistake in above strlen()
- X509_NAME *n = NULL;
- int nid;
+ /* maximum number of name elements */
+ max_ne = buflen / 2 + 1;
+
+ buf = malloc(buflen);
+ ne_types = malloc(max_ne);
+ ne_values = malloc(max_ne);
above two malloc() calls are inconsistent with original
code, which passed 'max_ne * sizeof(char *)' to malloc().
--patrick
USB *hci(4) freelists
In USB land there is a lot of freelists. Every HC driver has one
freelist per type of descriptor and the memory allocator used to
populate these lists, usb_allocmem(), is a wrapper around
bus_dma{mem,map}* that uses two more lists.
I don't see the point of having per-softc lists, but maybe somebody
has a better understanding of this.
I'd like to remove these lists and use usb_allocmem() directly. The
first reason for this change is to reduce the number of layers on top
of the memory allocator to track memleaks. Since I need to change the
way ehci(4)'s descriptor are allocated when a transfer is submitted,
this would definitively help.
This diff only removes the freelists for the descriptors used by
control, interrupt and bulk transfers. Isochronous will follow.
Ok?
Index: ehci.c
===
RCS file: /cvs/src/sys/dev/usb/ehci.c,v
retrieving revision 1.151
diff -u -p -r1.151 ehci.c
--- ehci.c 4 May 2014 14:42:36 - 1.151
+++ ehci.c 4 May 2014 14:44:08 -
@@ -2354,83 +2354,43 @@ struct ehci_soft_qh *
ehci_alloc_sqh(struct ehci_softc *sc)
{
struct ehci_soft_qh *sqh;
- usbd_status err;
- int i, offs;
struct usb_dma dma;
- if (sc-sc_freeqhs == NULL) {
- DPRINTFN(2, (ehci_alloc_sqh: allocating chunk\n));
- err = usb_allocmem(sc-sc_bus, EHCI_SQH_SIZE * EHCI_SQH_CHUNK,
- EHCI_PAGE_SIZE, dma);
-#ifdef EHCI_DEBUG
- if (err)
- printf(ehci_alloc_sqh: usb_allocmem()=%d\n, err);
-#endif
- if (err)
- return (NULL);
- for(i = 0; i EHCI_SQH_CHUNK; i++) {
- offs = i * EHCI_SQH_SIZE;
- sqh = KERNADDR(dma, offs);
- sqh-physaddr = DMAADDR(dma, offs);
- sqh-dma = dma;
- sqh-offs = offs;
- sqh-next = sc-sc_freeqhs;
- sc-sc_freeqhs = sqh;
- }
- }
- sqh = sc-sc_freeqhs;
- sc-sc_freeqhs = sqh-next;
- memset(sqh-qh, 0, sizeof(struct ehci_qh));
+ if (usb_allocmem(sc-sc_bus, sizeof(*sqh), EHCI_QH_ALIGN, dma))
+ return (NULL);
+
+ sqh = KERNADDR(dma, 0);
+ sqh-physaddr = DMAADDR(dma, 0);
+ sqh-dma = dma;
+ sqh-offs = 0;
sqh-next = NULL;
sqh-prev = NULL;
+ memset(sqh-qh, 0, sizeof(struct ehci_qh));
+
return (sqh);
}
void
ehci_free_sqh(struct ehci_softc *sc, struct ehci_soft_qh *sqh)
{
- sqh-next = sc-sc_freeqhs;
- sc-sc_freeqhs = sqh;
+ usb_freemem(sc-sc_bus, sqh-dma);
}
struct ehci_soft_qtd *
ehci_alloc_sqtd(struct ehci_softc *sc)
{
struct ehci_soft_qtd *sqtd;
- usbd_status err;
- int i, offs;
struct usb_dma dma;
- int s;
- if (sc-sc_freeqtds == NULL) {
- DPRINTFN(2, (ehci_alloc_sqtd: allocating chunk\n));
- err = usb_allocmem(sc-sc_bus, EHCI_SQTD_SIZE*EHCI_SQTD_CHUNK,
- EHCI_PAGE_SIZE, dma);
-#ifdef EHCI_DEBUG
- if (err)
- printf(ehci_alloc_sqtd: usb_allocmem()=%d\n, err);
-#endif
- if (err)
- return (NULL);
- s = splusb();
- for(i = 0; i EHCI_SQTD_CHUNK; i++) {
- offs = i * EHCI_SQTD_SIZE;
- sqtd = KERNADDR(dma, offs);
- sqtd-physaddr = DMAADDR(dma, offs);
- sqtd-dma = dma;
- sqtd-offs = offs;
- sqtd-nextqtd = sc-sc_freeqtds;
- sc-sc_freeqtds = sqtd;
- }
- splx(s);
- }
-
- s = splusb();
- sqtd = sc-sc_freeqtds;
- sc-sc_freeqtds = sqtd-nextqtd;
- memset(sqtd-qtd, 0, sizeof(struct ehci_qtd));
+ if (usb_allocmem(sc-sc_bus, sizeof(*sqtd), EHCI_QTD_ALIGN, dma))
+ return (NULL);
+
+ sqtd = KERNADDR(dma, 0);
+ sqtd-physaddr = DMAADDR(dma, 0);
+ sqtd-dma = dma;
+ sqtd-offs = 0;
sqtd-nextqtd = NULL;
- splx(s);
+ memset(sqtd-qtd, 0, sizeof(struct ehci_qtd));
return (sqtd);
}
@@ -2438,12 +2398,7 @@ ehci_alloc_sqtd(struct ehci_softc *sc)
void
ehci_free_sqtd(struct ehci_softc *sc, struct ehci_soft_qtd *sqtd)
{
- int s;
-
- s = splusb();
- sqtd-nextqtd = sc-sc_freeqtds;
- sc-sc_freeqtds = sqtd;
- splx(s);
+ usb_freemem(sc-sc_bus, sqtd-dma);
}
usbd_status
Index: ehcivar.h
===
RCS file: /cvs/src/sys/dev/usb/ehcivar.h,v
retrieving revision 1.31
diff -u -p -r1.31 ehcivar.h
--- ehcivar.h 29 Apr 2014 12:45:29 - 1.31
+++ ehcivar.h 4 May 2014 14:44:08 -
@@ -39,8 +39,6 @@ struct ehci_soft_qtd {
HID parser
In December 2012 a user reported on misc@ that the Noppoo Mini Choc
84 USB keyboard does not work on OpenBSD [0]. More recently, mcbride@
and yasuoka@ contacted me because they have a mouse that is not properly
recognized. Both issues are related to our HID descriptor parser.
The diff below is an updated version of a diff I sent one year ago that
I forgot because of the lack of feedbacks. Here's the original
description of the problem:
The descriptor [of the mouse/keyboard] includes an Item with an Usage
array (Min-Max range) *and* various other single Usage elements.
Unfortunately our HID parser doesn't handle such descriptor, so I
backported FreeBSD's updated version, see below.
I'd appreciate if people could test this diff and tell me if it breaks
something.
I'm also looking for Oks.
Martin
[0] http://marc.info/?l=openbsd-miscm=135287138512465w=2
Index: hid.c
===
RCS file: /cvs/src/sys/dev/usb/hid.c,v
retrieving revision 1.25
diff -u -p -r1.25 hid.c
--- hid.c 5 Aug 2012 16:07:11 - 1.25
+++ hid.c 4 May 2014 15:35:03 -
@@ -41,36 +41,52 @@
#include dev/usb/hid.h
-#ifdef UHIDEV_DEBUG
-#define DPRINTF(x) do { if (uhidevdebug) printf x; } while (0)
-#define DPRINTFN(n,x) do { if (uhidevdebug(n)) printf x; } while (0)
-extern int uhidevdebug;
+#if UHIDEV_DEBUG
+#define DPRINTFN(n,x...) do { if (uhidevdebug(n)) printf(x); } while (0)
+int uhidevdebug = 0;
#else
-#define DPRINTF(x)
-#define DPRINTFN(n,x)
+#define DPRINTFN(n,x...)
#endif
-void hid_clear_local(struct hid_item *);
+void hid_clear_local(struct hid_item *);
+uint8_thid_get_byte(struct hid_data *, const uint16_t);
+void hid_switch_rid(struct hid_data *, struct hid_item *, int32_t);
+
+#defineMAXUSAGE 64
+#defineMAXPUSH 4
+#defineMAXID 16
+
+struct hid_pos_data {
+ int32_t rid;
+ uint32_t pos;
+};
-#define MAXUSAGE 256
struct hid_data {
- u_char *start;
- u_char *end;
- u_char *p;
- struct hid_item cur;
- int32_t usages[MAXUSAGE];
- int nu;
- int minset;
- int multi;
- int multimax;
+ const uint8_t *start;
+ const uint8_t *end;
+ const uint8_t *p;
+ struct hid_item cur[MAXPUSH];
+ struct hid_pos_data last_pos[MAXID];
+ int32_t usages_min[MAXUSAGE];
+ int32_t usages_max[MAXUSAGE];
+ int32_t usage_last; /* last seen usage */
+ uint32_t loc_size; /* last seen size */
+ uint32_t loc_count; /* last seen count */
enum hid_kind kind;
+ uint8_t pushlevel; /* current pushlevel */
+ uint8_t ncount; /* end usage item count */
+ uint8_t icount; /* current usage item count */
+ uint8_t nusage; /* end usages_min/max index */
+ uint8_t iusage; /* current usages_min/max index */
+ uint8_t ousage; /* current usages_min/max offset */
+ uint8_t susage; /* usage set flags */
};
void
hid_clear_local(struct hid_item *c)
{
-
- DPRINTFN(5,(hid_clear_local\n));
+ c-loc.count = 0;
+ c-loc.size = 0;
c-usage = 0;
c-usage_minimum = 0;
c-usage_maximum = 0;
@@ -83,15 +99,62 @@ hid_clear_local(struct hid_item *c)
c-set_delimiter = 0;
}
+void
+hid_switch_rid(struct hid_data *s, struct hid_item *c, int32_t nextid)
+{
+ uint8_t i;
+
+ if (c-report_ID == nextid)
+ return;
+
+ /* save current position for current rID */
+ if (c-report_ID == 0) {
+ i = 0;
+ } else {
+ for (i = 1; i != MAXID; i++) {
+ if (s-last_pos[i].rid == c-report_ID)
+ break;
+ if (s-last_pos[i].rid == 0)
+ break;
+ }
+ }
+ if (i != MAXID) {
+ s-last_pos[i].rid = c-report_ID;
+ s-last_pos[i].pos = c-loc.pos;
+ }
+
+ /* store next report ID */
+ c-report_ID = nextid;
+
+ /* lookup last position for next rID */
+ if (nextid == 0) {
+ i = 0;
+ } else {
+ for (i = 1; i != MAXID; i++) {
+ if (s-last_pos[i].rid == nextid)
+ break;
+ if (s-last_pos[i].rid == 0)
+ break;
+ }
+ }
+ if (i != MAXID) {
+ s-last_pos[i].rid = nextid;
+ c-loc.pos = s-last_pos[i].pos;
+ } else {
+ DPRINTFN(0, Out of RID entries, position is set to zero!\n);
+ c-loc.pos = 0;
+ }
+}
+
struct hid_data *
-hid_start_parse(void *d, int len, enum hid_kind kind)
+hid_start_parse(const void *d, int len, enum hid_kind kind)
{
struct hid_data *s;
s = malloc(sizeof *s, M_TEMP, M_WAITOK | M_ZERO);
ums(4) memory leak
Plug a memory leak related to HID descriptor parsing, ok? Index: hidms.c === RCS file: /cvs/src/sys/dev/usb/hidms.c,v retrieving revision 1.5 diff -u -p -r1.5 hidms.c --- hidms.c 9 Aug 2013 22:10:17 - 1.5 +++ hidms.c 4 May 2014 15:56:47 - @@ -257,6 +257,7 @@ hidms_setup(struct device *self, struct break; } } + hid_end_parse(d); return 0; }
Re: malloc in libssl/src/apps
On Sun, May 04, 2014 at 12:17:16PM -0600, Theo de Raadt wrote:
We are going to completely ignore diffs which change multiple idioms
at once.
Okay.
That is how mistakes get made.
Yep, more true than I realized.
Here's a simpler one:
Index: apps.c
===
RCS file: /cvs/src/lib/libssl/src/apps/apps.c,v
retrieving revision 1.45
diff -u -p -r1.45 apps.c
--- apps.c 3 May 2014 16:03:54 - 1.45
+++ apps.c 4 May 2014 19:35:59 -
@@ -209,13 +209,10 @@ chopup_args(ARGS * arg, char *buf, int *
*argc = 0;
*argv = NULL;
- i = 0;
if (arg-count == 0) {
arg-count = 20;
- arg-data = (char **)malloc(sizeof(char *) * arg-count);
+ arg-data = calloc(arg-count, sizeof(char *));
}
- for (i = 0; i arg-count; i++)
- arg-data[i] = NULL;
num = 0;
p = buf;
@@ -232,8 +229,7 @@ chopup_args(ARGS * arg, char *buf, int *
if (num = arg-count) {
char **tmp_p;
int tlen = arg-count + 20;
- tmp_p = (char **) realloc(arg-data,
- sizeof(char *) * tlen);
+ tmp_p = reallocarray(arg-data, tlen, sizeof(char *));
if (tmp_p == NULL)
return 0;
arg-data = tmp_p;
@@ -1836,9 +1832,9 @@ parse_name(char *subject, long chtype, i
* only become shorter */
char *buf = malloc(buflen);
size_t max_ne = buflen / 2 + 1; /* maximum number of name elements */
- char **ne_types = malloc(max_ne * sizeof(char *));
- char **ne_values = malloc(max_ne * sizeof(char *));
- int *mval = malloc(max_ne * sizeof(int));
+ char **ne_types = reallocarray(NULL, max_ne, sizeof(char *));
+ char **ne_values = reallocarray(NULL, max_ne, sizeof(char *));
+ int *mval = reallocarray(NULL, max_ne, sizeof(int));
char *sp = subject, *bp = buf;
int i, ne_num = 0;
Index: ca.c
===
RCS file: /cvs/src/lib/libssl/src/apps/ca.c,v
retrieving revision 1.48
diff -u -p -r1.48 ca.c
--- ca.c2 May 2014 17:06:46 - 1.48
+++ ca.c4 May 2014 19:36:00 -
@@ -2002,8 +2002,8 @@ again2:
row[DB_type][0] = 'V';
row[DB_type][1] = '\0';
- if ((irow = (char **)malloc(sizeof(char *) * (DB_NUMBER + 1))) ==
- NULL) {
+ irow = reallocarray(NULL, DB_NUMBER + 1, sizeof(char *));
+ if (irow == NULL) {
BIO_printf(bio_err, Memory allocation failure\n);
goto err;
}
@@ -2267,8 +2267,8 @@ do_revoke(X509 * x509, CA_DB * db, int t
row[DB_type][0] = 'V';
row[DB_type][1] = '\0';
- if ((irow = (char **)malloc(sizeof(char *) *
- (DB_NUMBER + 1))) == NULL) {
+ irow = reallocarray(NULL, DB_NUMBER + 1, sizeof(char *));
+ if (irow == NULL) {
BIO_printf(bio_err, Memory allocation failure\n);
goto err;
}
Index: ecparam.c
===
RCS file: /cvs/src/lib/libssl/src/apps/ecparam.c,v
retrieving revision 1.10
diff -u -p -r1.10 ecparam.c
--- ecparam.c 24 Apr 2014 12:22:22 - 1.10
+++ ecparam.c 4 May 2014 19:36:00 -
@@ -312,7 +312,7 @@ bad:
crv_len = EC_get_builtin_curves(NULL, 0);
- curves = malloc((int) (sizeof(EC_builtin_curve) * crv_len));
+ curves = reallocarray(NULL, crv_len, sizeof(EC_builtin_curve));
if (curves == NULL)
goto end;
Index: speed.c
===
RCS file: /cvs/src/lib/libssl/src/apps/speed.c,v
retrieving revision 1.38
diff -u -p -r1.38 speed.c
--- speed.c 2 May 2014 17:06:46 - 1.38
+++ speed.c 4 May 2014 19:36:00 -
@@ -2178,7 +2178,7 @@ do_multi(int multi)
int *fds;
static char sep[] = :;
- fds = malloc(multi * sizeof *fds);
+ fds = reallocarray(NULL, multi, sizeof(int));
for (n = 0; n multi; ++n) {
if (pipe(fd) == -1) {
fprintf(stderr, pipe failure\n);
Index: srp.c
===
RCS file: /cvs/src/lib/libssl/src/apps/srp.c,v
retrieving revision 1.10
diff -u -p -r1.10 srp.c
--- srp.c 24 Apr 2014 12:22:22 - 1.10
+++ srp.c 4 May 2014 19:36:00 -
@@ -176,7 +176,8 @@ update_index(CA_DB * db, BIO * bio, char
char **irow;
int i;
- if ((irow = (char **) malloc(sizeof(char *) * (DB_NUMBER + 1))) ==
NULL) {
+ irow = reallocarray(NULL, DB_NUMBER + 1,
Re: vfs references to strncpy and MFSNAMELEN
I did not get any reply except yours.-
Indeed patching is very easy that's why I didn't bother to post one and let
most advanced users to patch it.-
Testing for regressions can be hard.-
I've found, in 5.4 version, upto 1400 references to strncpy but most of them
should cause no problem (nor overflow possible if they function which calls it
has the correct 'bounds').
As you correctly said I've missed that reference that expect a nul-terminating
string, however, if the change strncpy/strlcpy is done there should be no
problem to produce a nul-terminate string for the function you mention.
Best regards
-Original Message-
From: owner-t...@openbsd.org [mailto:owner-t...@openbsd.org] On Behalf Of
patrick keshishian
Sent: viernes, 02 de mayo de 2014 00:17
To: tech@openbsd.org; Héctor Luis Gimbatti
Subject: Re: vfs references to strncpy and MFSNAMELEN
On 4/29/14, Héctor Luis Gimbatti h...@etale.com.ar wrote:
The constant MFSNAMELEN as defined in:
lib/libc/sys/getfsstat.2:#define MFSNAMELEN 16
lib/libc/sys/statfs.2:#define MFSNAMELEN 16
sys/sys/mount.h: #define MFSNAMELEN 16
defines the fs type name and, according to comments, it includes nul
terminating character.
The following code makes uses of strncpy and involves MFSNAMELEN
./sys/kern/vfs_subr.c:225: strncpy(mp-mnt_stat.f_fstypename,
vfsp-vfc_name, MFSNAMELEN);
./sys/kern/vfs_subr.c:2272: strncpy(sbp-f_fstypename,
mp-mnt_vfc-vfc_name, MFSNAMELEN);
./sys/kern/vfs_syscalls.c:243: strncpy(mp-mnt_stat.f_fstypename,
vfsp-vfc_name, MFSNAMELEN);
./sys/miscfs/procfs/procfs_vfsops.c:190:strncpy(sbp-f_fstypename,
mp-mnt_vfc-vfc_name, MFSNAMELEN);
./sys/msdosfs/msdosfs_vfsops.c:669: strncpy(sbp-f_fstypename,
mp-mnt_vfc-vfc_name, MFSNAMELEN);
./sys/nfs/nfs_vfsops.c:646: strncpy(mp-mnt_stat.f_fstypename[0],
mp-mnt_vfc-vfc_name, MFSNAMELEN);
./sys/ntfs/ntfs_vfsops.c:628: strncpy(sbp-f_fstypename,
mp-mnt_vfc-vfc_name, MFSNAMELEN);
./sys/ufs/ext2fs/ext2fs_vfsops.c:704: strncpy(sbp-f_fstypename,
mp-mnt_vfc-vfc_name, MFSNAMELEN);
./sys/ufs/mfs/mfs_vfsops.c:222: strncpy(sbp-f_fstypename[0],
mp-mnt_vfc-vfc_name, MFSNAMELEN);
Can be those replace safely by strlcpy without any modification to
MFSNAMELEN constant?
I thought I had seen someone reply to this on tech@ but I can't find any
evidence of that. So... *ping*?
Most places use strn{cmp,cpy} except for compat/linux/linux_misc.c so it
seems OK. However, OP's grep missed the following usage which (I imagine)
expects a nul-terminated c-string:
sys/kern/vfs_subr.c in vfs_mount_print()
(*pr)( fstype \%s\ mnton \%s\ mntfrom \%s\ mntspec \%s\\n,
mp-mnt_stat.f_fstypename, mp-mnt_stat.f_mntonname,
mp-mnt_stat.f_mntfromname, mp-mnt_stat.f_mntfromspec);
Called from ddb/db_command.c: db_show_all_{mount,vnode}s()
Producing a patch as OP suggested is simple enough, but testing for regression
is a bit of an unknown to me.
--patrick
Re: malloc in libssl/src/apps
On 05/04/14 21:50, Jean-Philippe Ouellet wrote:
On Sun, May 04, 2014 at 12:17:16PM -0600, Theo de Raadt wrote:
We are going to completely ignore diffs which change multiple idioms
at once.
Okay.
That is how mistakes get made.
Yep, more true than I realized.
Here's a simpler one:
Index: apps.c
===
RCS file: /cvs/src/lib/libssl/src/apps/apps.c,v
retrieving revision 1.45
diff -u -p -r1.45 apps.c
--- apps.c 3 May 2014 16:03:54 - 1.45
+++ apps.c 4 May 2014 19:35:59 -
@@ -209,13 +209,10 @@ chopup_args(ARGS * arg, char *buf, int *
*argc = 0;
*argv = NULL;
- i = 0;
if (arg-count == 0) {
arg-count = 20;
- arg-data = (char **)malloc(sizeof(char *) * arg-count);
+ arg-data = calloc(arg-count, sizeof(char *));
}
- for (i = 0; i arg-count; i++)
- arg-data[i] = NULL;
General question; Given that NULL theoretically could be != 0, and that
we generally compare e.g. the malloc() result to NULL specifically, is
this approach acceptable?
/Alexander
Re: malloc in libssl/src/apps
On Sun, May 4, 2014 at 5:00 PM, Alexander Hall alexan...@beard.se wrote:
On 05/04/14 21:50, Jean-Philippe Ouellet wrote:
On Sun, May 04, 2014 at 12:17:16PM -0600, Theo de Raadt wrote:
We are going to completely ignore diffs which change multiple idioms
at once.
Okay.
That is how mistakes get made.
Yep, more true than I realized.
Here's a simpler one:
Index: apps.c
===
RCS file: /cvs/src/lib/libssl/src/apps/apps.c,v
retrieving revision 1.45
diff -u -p -r1.45 apps.c
--- apps.c 3 May 2014 16:03:54 - 1.45
+++ apps.c 4 May 2014 19:35:59 -
@@ -209,13 +209,10 @@ chopup_args(ARGS * arg, char *buf, int *
*argc = 0;
*argv = NULL;
- i = 0;
if (arg-count == 0) {
arg-count = 20;
- arg-data = (char **)malloc(sizeof(char *) * arg-count);
+ arg-data = calloc(arg-count, sizeof(char *));
}
- for (i = 0; i arg-count; i++)
- arg-data[i] = NULL;
General question; Given that NULL theoretically could be != 0, and that we
generally compare e.g. the malloc() result to NULL specifically, is this
approach acceptable?
The comparisons `NULL == 0` and `malloc() == 0` are *value* comparison.
After the statement `void **p = calloc(1, sizeof(void **));`, the
comparisons `p == NULL` and therefore `p == 0`, which are *identical*,
are not necessarily true.
Yes, to be correct you can't assume the representation is a string of
NULs, but the rest of your post is misleading.
/Alexander
Re: malloc in libssl/src/apps
On Sun, May 04, 2014 at 11:30:40PM +0200, Alexander Hall wrote: NULL theoretically could be != 0 Umm... short of something like: #undef NULL #define NULL I'm silly and want to break everything or something, I don't see when that'd be the case. According to ISO/IEC 9899:1999 TC3 (n1256) and the freely available draft of ISO/IEC 9899:201x (n1570), section 6.3.2.3, paragraph 3: [1] An integer constant expression with the value 0, or such an expression cast to type void *, is called a null pointer constant. If a null pointer constant is converted to a pointer type, the resulting pointer, called a null pointer, is guaranteed to compare unequal to a pointer to any object or function. So while it might not be cast to a pointer, it's still zero. And for what it's worth, POSIX does define it to be (void *)0 [2] The stddef.h header shall define the following macros: NULL Null pointer constant. The macro shall expand to an integer constant expression with the value 0 cast to type void *. [1] http://www.iso-9899.info/n1256.html#6.3.2.3p3 [2] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/stddef.h.html Did I miss something again?
Re: malloc in libssl/src/apps
On Mon, May 05, 2014 at 11:12:00AM +1000, Joel Sing wrote:
- i = 0;
if (arg-count == 0) {
arg-count = 20;
- arg-data = (char **)malloc(sizeof(char *) * arg-count);
+ arg-data = calloc(arg-count, sizeof(char *));
}
- for (i = 0; i arg-count; i++)
- arg-data[i] = NULL;
This one is a change in behaviour - if arg-count is 0 then previously we
zeroed arg-data; now we do not.
This one is calloc, not reallocarray, so unless I'm seriously missing
something obvious here, it is indeed zero'd, no?
Re: malloc in libssl/src/apps
On Sun, May 04, 2014 at 03:50:06PM -0400, Jean-Philippe Ouellet wrote:
On Sun, May 04, 2014 at 12:17:16PM -0600, Theo de Raadt wrote:
We are going to completely ignore diffs which change multiple idioms
at once.
Okay.
That is how mistakes get made.
Yep, more true than I realized.
Here's a simpler one:
[...]
Index: speed.c
===
RCS file: /cvs/src/lib/libssl/src/apps/speed.c,v
retrieving revision 1.38
diff -u -p -r1.38 speed.c
--- speed.c 2 May 2014 17:06:46 - 1.38
+++ speed.c 4 May 2014 19:36:00 -
@@ -2178,7 +2178,7 @@ do_multi(int multi)
int *fds;
static char sep[] = :;
- fds = malloc(multi * sizeof *fds);
+ fds = reallocarray(NULL, multi, sizeof(int));
how about a check for return value here?
similar to other malloc - reallocarray changes.
--patrick
for (n = 0; n multi; ++n) {
if (pipe(fd) == -1) {
fprintf(stderr, pipe failure\n);
Index: srp.c
===
RCS file: /cvs/src/lib/libssl/src/apps/srp.c,v
retrieving revision 1.10
diff -u -p -r1.10 srp.c
--- srp.c 24 Apr 2014 12:22:22 - 1.10
+++ srp.c 4 May 2014 19:36:00 -
@@ -176,7 +176,8 @@ update_index(CA_DB * db, BIO * bio, char
char **irow;
int i;
- if ((irow = (char **) malloc(sizeof(char *) * (DB_NUMBER + 1))) ==
NULL) {
+ irow = reallocarray(NULL, DB_NUMBER + 1, sizeof(char *));
+ if (irow == NULL)
BIO_printf(bio_err, Memory allocation failure\n);
return 0;
}
