Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Are fgetpos, fsetpos, fseeko and ftello broken with _FILE_OFFSET_BITS=64 when __ANDROID_API__ < 24? #442

Closed
fornwall opened this issue Jul 2, 2017 · 7 comments
Assignees
Labels
Milestone

Comments

@fornwall
Copy link

fornwall commented Jul 2, 2017

When using _FILE_OFFSET_BITS=64 with unified headers on 32-bit platforms <sys/types.h> declares off_t to be 64 bits, while otherwise these platforms uses a 32-bit off_t.

Then <stdio.h> declares:

typedef off_t fpos_t;
...
#if defined(__USE_FILE_OFFSET64) && __ANDROID_API__ >= __ANDROID_API_N__
int fgetpos(FILE*, fpos_t*) __RENAME(fgetpos64);
int fsetpos(FILE*, const fpos_t*) __RENAME(fsetpos64);
int fseeko(FILE*, off_t, int) __RENAME(fseeko64);
off_t ftello(FILE*) __RENAME(ftello64);
...
#else
int fgetpos(FILE*, fpos_t*);
int fsetpos(FILE*, const fpos_t*);
int fseeko(FILE*, off_t, int);
off_t ftello(FILE*);
...
#endif

When targeting 32-bit platforms pre-N (that is, __ANDROID_API__ >= __ANDROID_API_N__ is false), and using __USE_FILE_OFFSET64, won't this result in problems in that the compiler emits calls to fgetpos/fsetpos/fseeko/ftello with 64-bit off_t arguments while the implementations in libc expect off_t to be 32-bit?

@alexcohn
Copy link

alexcohn commented Jul 3, 2017

I am afraid it's a known bug not only with Unified Headers

@fornwall
Copy link
Author

fornwall commented Jul 3, 2017

I am afraid it's a known bug not only with Unified Headers

Before unified headers off_t/fpos_t was always 32 bit in size, so the headers never incorrectly declared that fgetpos/fsetpos/fseeko/ftello took 64 bit sized arguments, which can happen now when __USE_FILE_OFFSET64 is set with unified headers.

I guess it would be more correct to declare them as taking the 32-bit __kernel_off_t
in the case when defined(__USE_FILE_OFFSET64) && __ANDROID_API__ < __ANDROID_API_N__?

@alexcohn
Copy link

alexcohn commented Jul 3, 2017

I don't remember the details, but some talented developer succeeded to bend the headers for NDK r.11 so that pread() received a 64-bit position and we were all puzzled for a while why the API was only working as expected for offset=0.

@enh
Copy link
Contributor

enh commented Jul 5, 2017

this was changed by me in #333 as a follow up to #332.

looking back, i think we made the wrong decision on those bugs. (maybe by false analogy to #324 which still seems fine, albeit unrelated.) we should have said "this failure to compile is correct: you asked for _FILE_OFFSET_BITS=64 and then used a function that takes an off_t but isn't available in a 64-bit variant at the API level you're targeting". it's early in the morning on the first day of work for us this week, so i reserve the right to change my mind when i'm fully awake, but right now i read my checkin comment...

Only rename fgetpos/fsetpos/fseeko/ftello/funopen if we're N or newer.
Without this, setting __FILE_OFFSET_BITS to 64 and targeting pre-L
made these functions entirely unavailable.

...and think "yes, it does, as it should since versions that took off64_t didn't exist back then".

i think the fix here is to revert the two changes (the mmap.h one and the "everything else" one), and call the original bugs "working as intended". i honestly have no idea what we were thinking when we made these changes, so i'll make sure i get a second opinion first!

@enh enh self-assigned this Jul 5, 2017
@enh
Copy link
Contributor

enh commented Jul 5, 2017

it's much later in the day, and i still think we were insane to have made these changes. i have no idea what we were thinking. https://android-review.googlesource.com/429481 basically backs them out --- the problem of "my code doesn't compile because this function doesn't exist" is much better than "my code compiles but fails at run time because the function really didn't exist", and easier to debug!

hubot pushed a commit to aosp-mirror/platform_bionic that referenced this issue Jul 10, 2017
We had several bugs filed saying "if I set _FILE_OFFSET_BITS=64 when
targeting an API < L, various functions are missing". Instead of
saying "yes, they are", we quietly just modified the header files to
expose the non-64-bit variants. This makes no sense. We can't just say
"oh, yeah, we don't have a version of this function that agrees with
your calling code about how large off_t is, but here's a version that
doesn't: I'm sure it'll be fine".

_FILE_OFFSET_BITS=64 on Android LP32 has always been a game of chance,
but that game should be "are all the functions my code needs available
at compile time?", not "will my code actually work at run time?".

Bug: android/ndk#449
Bug: android/ndk#442
Bug: android/ndk#333
Bug: android/ndk#332
Bug: android/ndk#324
Test: builds
Change-Id: Ib095251d3e21e77ed50cc3575388107fecec4ecd
@enh enh added this to the r15c milestone Jul 18, 2017
@enh enh added the headers label Jul 18, 2017
@enh enh assigned DanAlbert and unassigned enh Jul 18, 2017
@koying
Copy link

koying commented Aug 4, 2017

I'm afraid this fix produced an unwanted side-effect in 15c if you #include <cstdio> with __USE_FILE_OFFSET64

/opt/android-dev/android-toolchain-arm-21-r15c/include/c++/4.9.x/cstdio:107:11: error: '::fgetpos' has not been declared
   using ::fgetpos;
           ^
/opt/android-dev/android-toolchain-arm-21-r15c/include/c++/4.9.x/cstdio:117:11: error: '::fsetpos' has not been declared
   using ::fsetpos;
           ^

@DanAlbert
Copy link
Member

(filed a new bug rather than continuing on this one, since it's not quite the same problem, #480)

hubot pushed a commit to chromium/crashpad that referenced this issue Oct 6, 2017
Chrome (and therefore mini_chromium) has always built with
_FILE_OFFSET_BITS=64, which is intended to enable a 64-bit off_t even
for 32-bit programs. However, support was never present in Android with
NDK traditional headers.

The new NDK unified headers do recognize _FILE_OFFSET_BITS=64 and enable
a 64-bit off_t, along with corresponding functions and system call
wrappers. However, no mmap() wrapper supporting a 64-bit off_t for
32-bit programs was available prior to API 21 (Android 5.0 “Lollipop”),
so when targeting older API levels, NDK headers do not proivde an mmap()
declaration. This avoids silently truncating 64-bit off_t values to 32
bits. NDK r15b did make such an mmap() wrapper available
(https://android.googlesource.com/platform/bionic/+/785b249df024), and
it did silently truncate, but this was removed for r15c
(https://android.googlesource.com/platform/bionic/+/00fedf587917).

How should this work if _FILE_OFFSET_BITS is set to 64 and recent
unified headers are in use?

The strategy employed here is to provide an mmap() declaration in
compat, with a 64-bit off_t. That mmap() will call to Bionic’s mmap64()
wrapper if available (it’s available since Android 5.0 “Lollipop”). If
unavailable, it implements the same logic that mmap64() does directly,
which predominantly involves calling the __mmap2() system call. Bionic
has always provided wrappers for __mmap2().

Additional reading:
https://android.googlesource.com/platform/bionic/+/0bfcbaf4d069/docs/32-bit-abi.md#is-32_bit-1
android/ndk#442

Bug: crashpad:30
Change-Id: I98c10e2eda773cb6f3d9eb8db9b8bfde43c885e7
Reviewed-on: https://chromium-review.googlesource.com/705674
Reviewed-by: Joshua Peraza <[email protected]>
inniyah added a commit to inniyah/AndroidTests that referenced this issue Aug 4, 2020
sepehrst pushed a commit to spsforks/android-bionic-libc that referenced this issue Apr 22, 2024
We had several bugs filed saying "if I set _FILE_OFFSET_BITS=64 when
targeting an API < L, various functions are missing". Instead of
saying "yes, they are", we quietly just modified the header files to
expose the non-64-bit variants. This makes no sense. We can't just say
"oh, yeah, we don't have a version of this function that agrees with
your calling code about how large off_t is, but here's a version that
doesn't: I'm sure it'll be fine".

_FILE_OFFSET_BITS=64 on Android LP32 has always been a game of chance,
but that game should be "are all the functions my code needs available
at compile time?", not "will my code actually work at run time?".

Bug: android/ndk#442
Bug: android/ndk#333
Bug: android/ndk#332
Bug: android/ndk#324
Test: builds
Change-Id: Ib095251d3e21e77ed50cc3575388107fecec4ecd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants