Bug 22965
| Summary: | [MFC] [PATCH] fix for minor bug in libc/gen/getcap.c | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Garance A Drosehn <gad> | ||||
| Component: | bin | Assignee: | Garance A Drosehn <gad> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Only Me | CC: | gad | ||||
| Priority: | Normal | ||||||
| Version: | 3.2-RELEASE | ||||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| Attachments: |
|
||||||
-*- Mode: BDE -*- <<On Sun, 19 Nov 2000 18:51:26 -0500 (EST), Garance A Drosehn <gad@freefour.acs.rpi.edu> said: > -static size_t topreclen; /* toprec length */ > -static char *toprec; /* Additional record specified by cgetset() */ > -static int gottoprec; /* Flag indicating retrieval of toprecord */ > +static size_t topreclen = 0; /* toprec length */ > +static char *toprec = NULL; /* Additional record specified by cgetset() */ > +static int gottoprec = 0; /* Flag indicating retrieval of toprecord */ These changes are erroneous, and have the sole effect of moving these variables from .bss, where they occupy only virtual space, to .data where they waste space on disk. > - (void)fclose(pfp); > - if (ferror(pfp)) { > + fcloseres = fclose(pfp); > + pfp = NULL; > + if (fcloseres != 0) { This also appears to be the Wrong Thing. The intent of the code seems to be to detect whether *reading* failed. As implemented in FreeBSD, fclose() of a read-only file can never fail. The correct emendation would be: haderror = ferror(pfp); fclose(pfp); if (haderror) { Even thus, this still could clobber errno (although fgetln() does not necessarily set it usefully); an even better version might be: int savederrno, haderror; savederrno = errno; /* ... */ haderror = ferror(pfp); if (haderror) savederrno = errno; fclose(pfp); if (haderror) { cgetclose(); errno = savederrno; return (-1); } -GAWollman -- Garrett A. Wollman | O Siem / We are all family / O Siem / We're all the same wollman@lcs.mit.edu | O Siem / The fires of freedom Opinions not those of| Dance in the burning flame MIT, LCS, CRS, or NSA| - Susan Aglukark and Chad Irschick Here is an improved version of the patch. Note that I have tested
this on a private copy of 'getcap.c' that I use in a project of
mine (which is compiled on multiple platforms), so I know it compiles
and it works fine in that project. However, I haven't figured out
how to build a new libc on freebsd, so I can't say I've tested this
patch installed into freebsd's libc...
Index: getcap.c
===================================================================
RCS file: /home/ncvs/src/lib/libc/gen/getcap.c,v
retrieving revision 1.12
diff -u -r1.12 getcap.c
--- getcap.c 2000/05/21 02:55:09 1.12
+++ getcap.c 2000/11/21 02:14:10
@@ -647,7 +647,7 @@
char **db_array;
{
size_t len;
- int status, i, done;
+ int done, hadreaderr, i, savederrno, status;
char *cp, *line, *rp, *np, buf[BSIZE], nbuf[BSIZE];
u_int dummy;
@@ -665,9 +665,14 @@
} else {
line = fgetln(pfp, &len);
if (line == NULL && pfp) {
- (void)fclose(pfp);
- if (ferror(pfp)) {
- (void)cgetclose();
+ hadreaderr = ferror(pfp);
+ if (hadreaderr)
+ savederrno = errno;
+ fclose(pfp);
+ pfp = NULL;
+ if (hadreaderr) {
+ cgetclose();
+ errno = savederrno;
return (-1);
} else {
if (*++dbp == NULL) {
@@ -724,9 +729,18 @@
} else { /* name field extends beyond the line */
line = fgetln(pfp, &len);
if (line == NULL && pfp) {
- (void)fclose(pfp);
- if (ferror(pfp)) {
- (void)cgetclose();
+ /* Name extends beyond the EOF! */
+ hadreaderr = ferror(pfp);
+ if (hadreaderr)
+ savederrno = errno;
+ fclose(pfp);
+ pfp = NULL;
+ if (hadreaderr) {
+ cgetclose();
+ errno = savederrno;
+ return (-1);
+ } else {
+ cgetclose();
return (-1);
}
} else
---
Garance Alistair Drosehn = gad@eclipse.acs.rpi.edu
Senior Systems Programmer (MIME & NeXTmail capable)
Rensselaer Polytechnic Institute; Troy NY USA
On Nov 20th, I (Garance) wrote: > Here is an improved version of the patch. Note that I have tested > this on a private copy of 'getcap.c' that I use in a project of > mine [...]. However, I haven't figured out how to build a new > libc on freebsd, so I can't say I've tested this patch installed > into freebsd's libc... Well, back then I was trying to compile just libc without doing a buildworld, because I didn't have the time to do a buildworld last week (for various reasons I won't go into here...). I couldn't figure out a good trick for that. I did do a buildworld Monday night, with this revised update included, and it compiled OK and I haven't had any problems with it. Should I commit the change to current now? Or should I, uh, do something else with it? (what?) --- Garance Alistair Drosehn = gad@eclipse.acs.rpi.edu Senior Systems Programmer (MIME & NeXTmail capable) Rensselaer Polytechnic Institute; Troy NY USA State Changed From-To: open->suspended Patch has been applied, awaiting MFC. Responsible Changed From-To: freebsd-bugs->gad I've written and committed the patch... State Changed From-To: suspended->closed The patch has been MFC'ed to RELENG_4 |
You probably don't want to know what the problem is under linux... However, the problem comes down to code which does: (void)fclose(pfp); if (ferror(pfp)) { ...do stuff... } According to the "single unix spec", the behavior of calling ANY "f-routine" on a closed stream is undefined. I take it to mean that's a bad idea. Certainly on linux it can trigger some major headaches. Also, I don't see what is to be gained by calling ferror in the above case. My change is just to USE the return code from fclose, instead of throwing it away, and have error-processing key off that instead of ferror. Fix: Here is the patch I propose. If no one objects to this, I can commit this patch sometime after thanksgiving. Or I would be more than happy if someone else wants to claim ownership of getcap, and they generate their own patch for it... :-) This also initializes global (static) variables to appropriate values, although that probably doesn't effect anything. How-To-Repeat: .na.