Bug 93545 - dbus did not support large-file for stat64
Summary: dbus did not support large-file for stat64
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: x86 (IA32) Linux (All)
: medium normal
Assignee: hongxu jia
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review?
Keywords: patch
Depends on:
Blocks:
 
Reported: 2015-12-31 05:52 UTC by hongxu jia
Modified: 2016-02-08 17:37 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
fixed patch (1.61 KB, patch)
2015-12-31 06:04 UTC, hongxu jia
Details | Splinter Review

Description hongxu jia 2015-12-31 05:52:18 UTC
While starting dbus-daemon on a 32-bit linux host and it invokes
fstat to load /etc/dbus-1/system.conf through NFS. If system.conf
was created with a large indoe number on 64-bit host. The above
fstat invoking failed. Here is the log of strace:
............
root@qemu3:~# df 
Filesystem                                                                                1K-blocks       Used  Available Use% Mounted on
10.0.2.2:/buildarea/raid0/hjia/wrlinux-x-master-1120/build-20151230-qemux86/export/dist 15623790592 8566374092 7057416500  55% /

root@qemu3:~# cat /proc/cmdline 
console=ttyS0,115200 ip=dhcp root=/dev/nfs nfsroot=10.0.2.2:/buildarea/raid0/hjia/wrlinux-x-master-1120/build-20151230-qemux86/export/dist,nfsvers=3,port=3349,mountprog=21411,nfsprog=11411,udp,mountport=3348 rw clock=pit oprofile.timer=1 TCF=1

root@qemu3:~# ls -i /etc/dbus-1/system.conf 
62318090305 /etc/dbus-1/system.conf

root@qemu3:~# strace /usr/bin/dbus-daemon --system --address=systemd: --nofork --nopidfile --systemd-activation
|open("/etc/dbus-1/system.conf", O_RDONLY) = 4
|fstat64(4, {st_mode=S_IFREG|0644, st_size=3340, ...}) = 0
|close(4) = 0
|close(3) = 0
|write(2, "Failed to start message bus: Fai"..., 109Failed to start message bus:
Failed to stat "/etc/dbus-1/system.conf": Value too large for defined data type
|) = 109
|exit_group(1) = ?
|+++ exited with 1 +++
............
Comment 1 hongxu jia 2015-12-31 05:53:27 UTC
In this situation, we should support large-file for stat64. Add marco
AC_SYS_LARGEFILE to do the detection at configure time. It can be disabled
by configuring with the `--disable-largefile' option.
Comment 2 hongxu jia 2015-12-31 06:04:37 UTC
Created attachment 120747 [details] [review]
fixed patch
Comment 3 Simon McVittie 2016-01-04 10:14:44 UTC
(In reply to hongxu jia from comment #1)
> In this situation, we should support large-file for stat64.

So is this something that needs to be cargo-culted into literally every piece of C code that can ever open a file? :-(
Comment 4 Ross Burton 2016-01-05 14:43:35 UTC
Isn't the massive complication with this that public data structures can potentially change size due to underlying types changing?
Comment 5 Simon McVittie 2016-01-05 15:15:43 UTC
(In reply to Ross Burton from comment #4)
> Isn't the massive complication with this that public data structures can
> potentially change size due to underlying types changing?

In general yes, but libdbus' public headers and APIs (dbus_*) have a policy of not exposing OS data-types other than the very basic ones (<stddef.h>, <stdarg.h>). In particular, no struct stat, and no FILE *.

Internal stuff (_dbus_*) potentially do use OS data types like struct stat, but we don't have any ABI guarantees for those anyway.
Comment 6 Ross Burton 2016-01-07 17:21:07 UTC
That's good news, thanks Simon.
Comment 7 Simon McVittie 2016-02-08 14:21:37 UTC
Comment on attachment 120747 [details] [review]
fixed patch

Review of attachment 120747 [details] [review]:
-----------------------------------------------------------------

This commit message is a couple of orders of magnitude larger than the actual change :-)

Something like this would be sufficient to explain what you did and why:

> configure.ac: support large-file for stat64
>
> dbus-daemon is not expected to open files with large *sizes*, but without
> large file support, calling [f]stat() on a file that happens to have a large
> inode number will fail with EOVERFLOW (see stat(2)). For example, files
> mounted from an NFS server might have large inode numbers.

You don't need to restate the content of the patch in the commit message: I can tell you used the AC_SYS_LARGEFILE macro by looking at the actual patch :-)

Please be careful with the spelling of technical terms: "inode", "macro".
Comment 8 Simon McVittie 2016-02-08 15:55:49 UTC
Comment on attachment 120747 [details] [review]
fixed patch

Review of attachment 120747 [details] [review]:
-----------------------------------------------------------------

::: configure.ac
@@ +68,1 @@
>  AC_USE_SYSTEM_EXTENSIONS

The documentation of AC_USE_SYSTEM_EXTENSIONS says

> This should be called before any macros that run the C
> compiler.

and Autoconf issues warnings if this isn't true, which it isn't in your patch. I'll swap those two lines and commit.

Please verify that each new patch either does not introduce any new warnings, or introduces only new warnings for which you can justify why the warning is a false-positive and does not indicate anything harmful.
Comment 9 Simon McVittie 2016-02-08 17:37:32 UTC
Fixed in git for 1.10.8 and 1.11.2, thanks.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.