Re: Review of changes for getnetgrent.c
On May 21, 2012, at 2:29 PM, Xin Li wrote:
> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA256
>
> On 05/21/12 12:02, Guy Helmer wrote:
>>
>> On May 18, 2012, at 6:09 PM, Xin Li wrote:
>>
>>> -BEGIN PGP SIGNED MESSAGE- Hash: SHA256
>>>
>>> On 05/18/12 14:58, Guy Helmer wrote:
To close PR bin/83340, I have this change worked up to resolve
memory allocation failure handling and avoid creating bad
entries in the grp list due to memory allocation failures while
building a new entry.
Before committing, I wanted to run it past others to see if
there were any problems with it.
>>>
>>> %%% @@ -477,6 +475,13 @@ if (len > 0) { grp->ng_str[strpos] =
>>> (char *) malloc(len + 1); + if
>>> (grp->ng_str[strpos] == NULL)
>>> { + for (freepos = 0;
>>> freepos < strpos; freepos++) +
>>> if (grp->ng_str[freepos] != NULL) +
>>> free(grp->ng_str[freepos]); +
>>> free(grp); +
>>> return(1); +} bcopy(spos,
>>> grp->ng_str[strpos], len + 1);
>>> %%%
>>
>> Like this?
>>
>> if (len > 0) { grp->ng_str[strpos] = (char *) malloc(len + 1); +
>> if (grp->ng_str[strpos] == NULL) { +
>> int freepos; + for
>> (freepos = 0; freepos < strpos; freepos++) +
>> free(grp->ng_str[freepos]); +
>> free(grp); +
>> return(1); +
>> } bcopy(spos, grp->ng_str[strpos], len + 1); }
>>>
>>> There are a few return without space between the keyword and
>>> return value.
>>
>> Do you recommend I fix all those instances in the file, or just the
>> instances in this patch?
>
> I'd recommend fixing them all (note that you could run into a bigger
> commit as the switch() is not style(9) conformant at this time) and we
> normally do it in two different commits (one style, and another
> functional) when possible.
>
OK, thank you.
Guy
This message has been scanned by ComplianceSafe, powered by Palisade's
PacketSure.
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: Review of changes for getnetgrent.c
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256
On 05/21/12 12:02, Guy Helmer wrote:
>
> On May 18, 2012, at 6:09 PM, Xin Li wrote:
>
>> -BEGIN PGP SIGNED MESSAGE- Hash: SHA256
>>
>> On 05/18/12 14:58, Guy Helmer wrote:
>>> To close PR bin/83340, I have this change worked up to resolve
>>> memory allocation failure handling and avoid creating bad
>>> entries in the grp list due to memory allocation failures while
>>> building a new entry.
>>>
>>> Before committing, I wanted to run it past others to see if
>>> there were any problems with it.
>>
>> %%% @@ -477,6 +475,13 @@ if (len > 0) { grp->ng_str[strpos] =
>> (char *) malloc(len + 1); + if
>> (grp->ng_str[strpos] == NULL)
>> { + for (freepos = 0;
>> freepos < strpos; freepos++) +
>> if (grp->ng_str[freepos] != NULL) +
>> free(grp->ng_str[freepos]); +
>> free(grp); +
>> return(1); + } bcopy(spos,
>> grp->ng_str[strpos], len + 1);
>> %%%
>
> Like this?
>
> if (len > 0) { grp->ng_str[strpos] = (char *) malloc(len + 1); +
> if (grp->ng_str[strpos] == NULL) { +
> int freepos; + for
> (freepos = 0; freepos < strpos; freepos++) +
> free(grp->ng_str[freepos]); +
> free(grp); +return(1); +
> } bcopy(spos, grp->ng_str[strpos], len + 1); }
>>
>> There are a few return without space between the keyword and
>> return value.
>
> Do you recommend I fix all those instances in the file, or just the
> instances in this patch?
I'd recommend fixing them all (note that you could run into a bigger
commit as the switch() is not style(9) conformant at this time) and we
normally do it in two different commits (one style, and another
functional) when possible.
Cheers,
- --
Xin LI https://www.delphij.net/
FreeBSD - The Power to Serve! Live free or die
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.19 (FreeBSD)
iQEcBAEBCAAGBQJPupePAAoJEG80Jeu8UPuz52wH/RVJXpCyea+ep08XDx82D7tG
us+ujKa1aNOUumzwJRsJ4SNVBiyc+hqCtb8s7FjjeF4/SJk8oei/I1/M1JIyMuIh
FawSB8rNJCbn/u9Od19iOeh/f/IDeCN+q8OrUK5mqQ7G1KDcHs12h86AFlm9HA7K
8UyxneTkPfKhED6hkgSll6bqYAJLeR5jJ3CCGvBeXxNgzJyyAhICWv0UgzUpcY9d
l2beuIXc57toDaLrbWkooLfQclDWPWyyPXq7okexQAq8OUjqmQFE+EhcYsIbtBkH
uBW67jhH81MZf/Ryl83VeqT9IChOySAU0YiwOQxaxdlqR53VAenAY0sWS1QvuX8=
=drgy
-END PGP SIGNATURE-
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: Review of changes for getnetgrent.c
On May 18, 2012, at 6:09 PM, Xin Li wrote:
> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA256
>
> On 05/18/12 14:58, Guy Helmer wrote:
>> To close PR bin/83340, I have this change worked up to resolve
>> memory allocation failure handling and avoid creating bad entries
>> in the grp list due to memory allocation failures while building a
>> new entry.
>>
>> Before committing, I wanted to run it past others to see if there
>> were any problems with it.
>
> %%%
> @@ -477,6 +475,13 @@
> if (len > 0) {
> grp->ng_str[strpos] = (char *)
> malloc(len + 1);
> + if (grp->ng_str[strpos] ==
> NULL) {
> + for (freepos = 0;
> freepos < strpos; freepos++)
> + if
> (grp->ng_str[freepos] != NULL)
> +
> free(grp->ng_str[freepos]);
> + free(grp);
> + return(1);
> + }
> bcopy(spos, grp->ng_str[strpos],
> len + 1);
> %%%
Like this?
if (len > 0) {
grp->ng_str[strpos] = (char *)
malloc(len + 1);
+ if (grp->ng_str[strpos] ==
NULL) {
+ int freepos;
+ for (freepos = 0;
freepos < strpos; freepos++)
+
free(grp->ng_str[freepos]);
+ free(grp);
+ return(1);
+ }
bcopy(spos, grp->ng_str[strpos],
len + 1);
}
>
> There are a few return without space between the keyword and return value.
Do you recommend I fix all those instances in the file, or just the instances
in this patch?
Thanks,
Guy
This message has been scanned by ComplianceSafe, powered by Palisade's
PacketSure.
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: Review of changes for getnetgrent.c
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256
On 05/18/12 14:58, Guy Helmer wrote:
> To close PR bin/83340, I have this change worked up to resolve
> memory allocation failure handling and avoid creating bad entries
> in the grp list due to memory allocation failures while building a
> new entry.
>
> Before committing, I wanted to run it past others to see if there
> were any problems with it.
%%%
@@ -477,6 +475,13 @@
if (len > 0) {
grp->ng_str[strpos] = (char *)
malloc(len + 1);
+ if (grp->ng_str[strpos] ==
NULL) {
+ for (freepos = 0;
freepos < strpos; freepos++)
+ if
(grp->ng_str[freepos] != NULL)
+
free(grp->ng_str[freepos]);
+ free(grp);
+ return(1);
+ }
bcopy(spos, grp->ng_str[strpos],
len + 1);
%%%
The if (grp->ng_str[freepos] != NULL) here seems to be unnecessary to
me because in most cases these are false, and free() does the test
regardless. Also, I think freepos can be declared within this scope
level.
There are a few return without space between the keyword and return value.
Other than these it looks fine to me. Thanks for working on this!
Cheers,
- --
Xin LI https://www.delphij.net/
FreeBSD - The Power to Serve! Live free or die
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.19 (FreeBSD)
iQEcBAEBCAAGBQJPttaYAAoJEG80Jeu8UPuzFEsH/i7JwIPdk15sP3E2/YpesYQu
veavnS6tebylFhniKukN4GRsS5mpbs9AmnxbNUBfF7InlzcnxOeUX9mHJepxbZQX
Bz8wgsvfxlrrseIyscdwm7b4XQK3dLv+VwpbQ4fqACOX1kGEQ/GsIc65JLyp2Gzo
fRLkHRAO5s3FVT5f11vsy2Ry16AmQhL2Sg9+mrGqeIbhppmDCgWfoF+rmD/7fn15
0OuJNn/S3Cz3zo+ZHI9OE1W8mkMox4kznQmv6vH/hM3Gk1cY9h66UybuBWsY31dI
WF5Y5WoJBuncFlDxGkaZv2jiRAqgkfWILVWKcjyejtGgVYPEWAjIgHLyyVk7H4g=
=ewU+
-END PGP SIGNATURE-
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Review of changes for getnetgrent.c
To close PR bin/83340, I have this change worked up to resolve memory
allocation failure handling and avoid creating bad entries in the grp list due
to memory allocation failures while building a new entry.
Before committing, I wanted to run it past others to see if there were any
problems with it.
Thanks,
Guy
> svn diff lib/libc/gen
Index: lib/libc/gen/getnetgrent.c
===
--- lib/libc/gen/getnetgrent.c (revision 235606)
+++ lib/libc/gen/getnetgrent.c (working copy)
@@ -203,9 +203,7 @@
if (parse_netgrp(group))
endnetgrent();
else {
- grouphead.grname = (char *)
- malloc(strlen(group) + 1);
- strcpy(grouphead.grname, group);
+ grouphead.grname = strdup(group);
}
if (netf)
fclose(netf);
@@ -417,7 +415,7 @@
parse_netgrp(const char *group)
{
char *spos, *epos;
- int len, strpos;
+ int len, strpos, freepos;
#ifdef DEBUG
int fields;
#endif
@@ -454,9 +452,9 @@
while (pos != NULL && *pos != '\0') {
if (*pos == '(') {
grp = (struct netgrp *)malloc(sizeof (struct netgrp));
+ if (grp == NULL)
+ return(1);
bzero((char *)grp, sizeof (struct netgrp));
- grp->ng_next = grouphead.gr;
- grouphead.gr = grp;
pos++;
gpos = strsep(&pos, ")");
#ifdef DEBUG
@@ -477,6 +475,13 @@
if (len > 0) {
grp->ng_str[strpos] = (char *)
malloc(len + 1);
+ if (grp->ng_str[strpos] ==
NULL) {
+ for (freepos = 0;
freepos < strpos; freepos++)
+ if
(grp->ng_str[freepos] != NULL)
+
free(grp->ng_str[freepos]);
+ free(grp);
+ return(1);
+ }
bcopy(spos, grp->ng_str[strpos],
len + 1);
}
@@ -490,6 +495,8 @@
grp->ng_str[strpos] = NULL;
}
}
+ grp->ng_next = grouphead.gr;
+ grouphead.gr = grp;
#ifdef DEBUG
/*
* Note: on other platforms, malformed netgroup
@@ -526,7 +533,7 @@
static struct linelist *
read_for_group(const char *group)
{
- char *pos, *spos, *linep, *olinep;
+ char *pos, *spos, *linep;
int len, olen;
int cont;
struct linelist *lp;
@@ -534,6 +541,7 @@
#ifdef YP
char *result;
int resultlen;
+ linep = NULL;
while (_netgr_yp_enabled || fgets(line, LINSIZ, netf) != NULL) {
if (_netgr_yp_enabled) {
@@ -554,6 +562,7 @@
free(result);
}
#else
+ linep = NULL;
while (fgets(line, LINSIZ, netf) != NULL) {
#endif
pos = (char *)&line;
@@ -576,8 +585,14 @@
pos++;
if (*pos != '\n' && *pos != '\0') {
lp = (struct linelist *)malloc(sizeof (*lp));
+ if (lp == NULL)
+ return(NULL);
lp->l_parsed = 0;
lp->l_groupname = (char *)malloc(len + 1);
+ if (lp->l_groupname == NULL) {
+ free(lp);
+ return(NULL);
+ }
bcopy(spos, lp->l_groupname, len);
*(lp->l_groupname + len) = '\0';
len = strlen(pos);
@@ -595,15 +610,15 @@
} else
cont = 0;
if (len > 0) {
- linep = (char *)malloc(olen + len + 1);
- if (olen > 0) {
- bcopy(olinep, linep, olen);
- free(olinep);
+ linep = (char *)reallocf(linep, olen +
len + 1);
+
