openssh bugs - Sep 2015 - [Bug 2463] New: Conflict with openbsd compat glob() function in shared libraries

https://bugzilla.mindrot.org/show_bug.cgi?id=2463

            Bug ID: 2463
           Summary: Conflict with openbsd compat glob() function in shared
                    libraries
           Product: Portable OpenSSH
           Version: 7.1p1
          Hardware: Other
                OS: Linux
            Status: NEW
          Severity: enhancement
          Priority: P5
         Component: Miscellaneous
          Assignee: unassigned-bugs at mindrot.org
          Reporter: jjelen at redhat.com

Created attachment 2701
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2701&action=edit
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 at openssh.com, fstatvfs at 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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2463

Jakub Jelen <jjelen at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2701|0                           |1
        is obsolete|                            |

--- Comment #1 from Jakub Jelen <jjelen at redhat.com> ---
Created attachment 2704
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2704&action=edit
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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2463

Jakub Jelen <jjelen at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2468


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=2468
[Bug 2468] Option to include external files to sshd_config
-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2463

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2704|                            |ok-
              Flags|                            |

--- Comment #2 from Darren Tucker <dtucker at zip.com.au> ---
Comment on attachment 2704
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2704
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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2463

Jakub Jelen <jjelen at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2704|0                           |1
        is obsolete|                            |

--- Comment #3 from Jakub Jelen <jjelen at redhat.com> ---
Created attachment 2707
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2707&action=edit
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.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2463

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2707|0                           |1
        is obsolete|                            |
                 CC|                            |dtucker at zip.com.au

--- Comment #4 from Darren Tucker <dtucker at zip.com.au> ---
Created attachment 2708
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2708&action=edit
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?

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2463

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2708|0                           |1
        is obsolete|                            |
           Assignee|unassigned-bugs at mindrot.org |dtucker at zip.com.au

--- Comment #5 from Darren Tucker <dtucker at zip.com.au> ---
Created attachment 2709
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2709&action=edit
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.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2463

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |djm at mindrot.org
   Attachment #2709|                            |ok?(djm at mindrot.org)
              Flags|                            |

--- Comment #6 from Darren Tucker <dtucker at zip.com.au> ---
Comment on attachment 2709
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2709
shrink diff further by using #defines in openbsd-compat/glob.c

Jakub, could you please confirm that patch #2709 resolves the issue for
you?

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2463

--- Comment #7 from Jakub Jelen <jjelen at redhat.com> ---
Yes. It compiles fine and I briefly tested the case that was failing
and it works fine now.
Thank you!

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2463

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2709|ok?(djm at mindrot.org)        |ok+
              Flags|                            |

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2463

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #8 from Darren Tucker <dtucker at zip.com.au> ---
Patch applied and will be in 7.2p1.  Thanks.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2463

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #9 from Damien Miller <djm at mindrot.org> ---
Close all resolved bugs after 7.3p1 release

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.