Created attachment 2701 [details] proof of concept with patch I am really wondering if nobody ever hit this issue before so I apologize that this report will be a bit longer, because I would like to express the state of things and the way how I get to this issue. Short story is that we should not override this library function in the same "namespace" where are library functions, if we are not sure the functions and types are binary compatible (on Linux, glob_t is NOT), because other shared object can expect structure with different content. The long story: Currently glob function is used only in sftp.c, and sftp-glob.c files which build together into sftp binary, where is also "packed" compat function glob making the system glob() function not load (#define _GLOB_H_). This binary is quite standalone and doesn't issue many calls to different libraries so the ABI compatibility is not a big deal. But as I was working in recent time on Include feature, which requires some globbing, I hit this issue: SSHD started crashing in kerberos gssapi library after return from glob() call. The returned structure looked malformed and it took me some time to realize that non system, but openbsd-compat glob() was called, which is ... suboptimal ... This didn't appear before, since glob was not used in this binary and the compat glob() function was optimized out I believe. This explained the problem, but searching for solution was also painful. If I am right, openBSD is the only platform with glob with all these features (GLOB_HAS_GL_MATCHC, GLOB_HAS_GL_STATV are missing on Linux if I am right): * Considering writing ifdefs around every usage of these special features would be possibility, but the result would miss some features (statvfs@openssh.com, fstatvfs@openssh.com?) and code would be much more messy * Modification of glob_t structure that it would be ABI compatible with system one is also not much portable. My current solution is to redefine glob and related structures with some prefix (ex. compat_glob(), COMPAT_GLOB_NOMATCH) and also modify sftp to use these prefixed functions and constants not to interfere with system function (see attached patch, currently not portable). The result builds and works just fine. For portability reasons (there can be platform supporting all the extensions) I believe we can create constants (#define _GLOB_PREFIX compat_ + something for constatns) and use it as a prefix for these calls. I can elaborate later. But also I am open to other ideas how you would solve this issue in portable way. I guess I ran out of ideas at the moment.
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