| Summary: | Conflict with openbsd compat glob() function in shared libraries | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | Portable OpenSSH | Reporter: | Jakub Jelen <jjelen> | ||||||||||||
| Component: | Miscellaneous | Assignee: | Darren Tucker <dtucker> | ||||||||||||
| Status: | CLOSED FIXED | ||||||||||||||
| Severity: | enhancement | CC: | djm, dtucker | ||||||||||||
| Priority: | P5 | ||||||||||||||
| Version: | 7.1p1 | ||||||||||||||
| Hardware: | Other | ||||||||||||||
| OS: | Linux | ||||||||||||||
| Bug Depends on: | |||||||||||||||
| Bug Blocks: | 2468 | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Jakub Jelen
2015-09-12 01:38:33 AEST
Created attachment 2704 [details]
reworked patch in cleaner way
I reworked the patch a bit to make sure it will work with system glob, if it is available with all features.
"all tests passed" in testsuite so I believe there is no regression. But maybe there is better way, I don't know about, to achieve the same.
Comment on attachment 2704 [details] reworked patch in cleaner way > int > remote_glob(struct sftp_conn *conn, const char *pattern, int flags, >- int (*errfunc)(const char *, int), glob_t *pglob) >+ int (*errfunc)(const char *, int), GLOB_COMPAT(glob_t) *pglob) Requiring changes like that to the mainline code will be an ongoing maintenance disaster. Lots of diffs won't apply when syncing changes, and there's the risk one *does* but doesn't get the required change, becoming a potential landmine for someone to stumble over later. I'd rather see this done with the preprocessor only and in the compat code only. As long as nothing pulls in the system glob.h, something along the lines of this ought to work: #define glob_t compat_glob_t #define glob(a, b, c, d) __compat_glob((a), (b), (c), (d)) and you may not even need to #define the flags, maybe checking they're not already defined (ie nothing picked up the system glob.h) may be sufficient. Created attachment 2707 [details]
patch reworked as proposed
Thank you for comments. I knew there will be better and more maintainable way to do so. I reworked the patch as advised and it works well. I hope I got it right :)
Keeping the GLOB_* constants in whole openbsd-compat.h would make problems since it is included everywhere and they have totally different numerical values (for example) on linux than on openbsd.
As a resolution I moved glob include to sftp-client.h, which is only place where we need compat_glob. The other places requiring glob should include system glob on their own.
Created attachment 2708 [details]
shrink diff further by using #defines in openbsd-compat/glob.c
That's certainly better. There's a couple of other ways to improve it:
- the glob.c code will probably need syncing at some point too, albeit at a much lower rate than the mainline code. Using the #defines in there too removes a number diff lines.
- I'd prefer the decision to use the system glob or not be taken early so that the logic remains central if globs appear elsewhere. (It's also the place we'd first look if glob appears in some other code).
This seems to build warning-free and behave as expected on Linux using the compat libary:
$ nm sftp | grep _compat_glob
00015570 T _compat_glob
00015800 T _compat_globfree
and OpenBSD using the system library:
$ nm sftp | grep "U glob"
U glob
U globfree
Does this work for you?
Created attachment 2709 [details]
shrink diff further by using #defines in openbsd-compat/glob.c
Actually we have a precedent of using _ssh_compat_ as the prefix so changed to that. No other changes.
Comment on attachment 2709 [details]
shrink diff further by using #defines in openbsd-compat/glob.c
Jakub, could you please confirm that patch #2709 resolves the issue for you?
Yes. It compiles fine and I briefly tested the case that was failing and it works fine now. Thank you! Patch applied and will be in 7.2p1. Thanks. Close all resolved bugs after 7.3p1 release |