Bug 450011

Created attachment 308398 [details]
First draft implementation

Some hand holding and sanity checking would be very much welcome:

Here's a first draft implementation; in short, search for exit and _exit from
readelf -s output for shared libs in lib/lib64 paths.  The patch is against
upstream svn, should apply to rpmlint 0.83 (in Rawhide only at the moment).

While testing this patch, I see that quite a few cases that now trigger a
warning are ones where fork() is involved, for example F-8 gamin and
esound-libs.  Should we try to detect these cases and not trigger the warning
for them?

Should we check all shared objects, not just ones in lib/lib64 paths?  For
example apps' "private" DSO modules?

Could you submit a descriptive info message for rpmlint -i/-I that Joe Average
packagers would understand and that would help them get the problem fixed? 
"Because it's unspeakably rude" doesn't quite cut it, I think :)

Bill, could you take a look at comment 1 when you find time?

(In reply to comment #1)
> Here's a first draft implementation; in short, search for exit and _exit from
> readelf -s output for shared libs in lib/lib64 paths.  The patch is against
> upstream svn, should apply to rpmlint 0.83 (in Rawhide only at the moment).

I tested in rawhide.

> While testing this patch, I see that quite a few cases that now trigger a
> warning are ones where fork() is involved, for example F-8 gamin and
> esound-libs.  Should we try to detect these cases and not trigger the warning
> for them?

The more I think about it, if we can't filter these cases, we may want to throw
the idea out, because we'll get way too many false positives.

> Should we check all shared objects, not just ones in lib/lib64 paths?  For
> example apps' "private" DSO modules?

Probably not.

> Could you submit a descriptive info message for rpmlint -i/-I that Joe Average
> packagers would understand and that would help them get the problem fixed? 
> "Because it's unspeakably rude" doesn't quite cut it, I think :)

...
This library package calls exit() or _exit(). Doing so from a library is
strongly discouraged - when a library function calls exit(), it prevents the
calling program from handling the error, reporting it to the user, closing files
properly, and cleaning up any state that the program has. It is preferred for
the library to return an actual error code and let the calling program
decide how to handle the situation.

...

Created attachment 312820 [details]
2nd draft

Ok, thanks.  Here's the 2nd draft; this one quite naively omits all exit()
warnings if fork() is also called (anywhere) by the same shared lib.  This will
most likely let more problematic situations pass silently, but could be useful
nevertheless.  At least the warnings for the few packages I tried this with did
not look obvious false positives to me.  Thoughts?

Looks good. Would be mildly curious how many packages in rawhide this affects,
but I don't have a full mirror handy at the moment; may try tomorrow.

Forbidding all usage cases for exit(2) and _exit(2) in shared objects is rather
rude & crude behavior as well. There are certainly "can't happen" conditions
in libraries that justify linking exit/_exit symbols.

E.g.
$ nm /usr/lib/librpm-5.0.so | grep exit
         U _exit@@GLIBC_2.0
         U exit@@GLIBC_2.0

But whatever, --fuzz=0 is equally risky build policy to inflict on unsuspecting package monkeys imho 
...

Funny, that's one of the rudely behaved libaries that this is intended to flag....

Ha ha ...

You have to look at the usage case, malloc returning NULL is a "can't happen"
condition where an exit call is arguably justified.

Returning an error from library to application when malloc returns NULL assumes:

1) error return paths exist (not true for many paths in rpmlib).

2) applications are prepared to do something meaningful with the error (rpm-python
script kiddies ain't the best programmers).

> Returning an error from library to application when malloc returns NULL assumes:
> 
> 1) error return paths exist (not true for many paths in rpmlib).

Other sloppiness is no justification.

> 2) applications are prepared to do something meaningful with the error (rpm-python
> script kiddies ain't the best programmers).

And by calling exit(), you blindly make the assumption for any apps that may use
the library.

In any case, this bug isn't about the sanity or lack thereof of librpm.

"Just return an error." is no solution when stable ABI's are equally important.

DV hounded every occurence of exit out of rpmlib years ago. Surely you remember
the tedium on internal IRC ...

And no, the problem is quite general, you will find many developers that have
sloppy code paths and that blindly call exit/_exit if you pursue a packaging
policy of
    No library should call exit/_exit *EVER*.
That is clearly both unattainable perfection at the same time it is rude and crude
policy *AND* development. I'm using rpmlib only as an example.

Have fun!

I'm having a fine time. You seem to have misinterpreted a clearly specified
*warning* as a unilateral packaging policy.

Warning abt issues that can't be easily fixed serves about as much purpose as
discussing same issues in bugzilla.You could just as easily detect all libraries
that call exit, vet the results, triage into what issues are fixable (or not), as insturment
the detection into rpmlint. Comment #5 indicates you know that approach quite well.

No matter what: You are clearly a consenting adult, if *warning* is your fig leaf, go fer it!

Committed upstream, http://rpmlint.zarb.org/cgi-bin/trac.cgi/changeset/1448

rpmlint-0.85-1.fc9 has been submitted as an update for Fedora 9.
http://admin.fedoraproject.org/updates/rpmlint-0.85-1.fc9

rpmlint-0.85-1.fc9 has been pushed to the Fedora 9 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update rpmlint'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F9/FEDORA-2008-9125

rpmlint-0.85-2.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.

rpmlint-0.85-2.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/rpmlint-0.85-2.fc10

rpmlint-0.85-2.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.