Bug 11455 - problem in tdb 1.3.7 and ldb 1.1.21 in rep_strtoll in lib/replace/replace.c
Summary: problem in tdb 1.3.7 and ldb 1.1.21 in rep_strtoll in lib/replace/replace.c
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB (show other bugs)
Version: 4.2.3
Hardware: x64 FreeBSD
: P5 critical (vote)
Target Milestone: 4.2
Assignee: Stefan Metzmacher
QA Contact: Samba QA Contact
URL:
Keywords:
: 11497 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-08-18 07:52 UTC by Dron
Modified: 2021-01-18 11:09 UTC (History)
5 users (show)

See Also:


Attachments
Patch (806 bytes, patch)
2015-08-18 19:14 UTC, Volker Lendecke
no flags Details
Patch for v4-3-test (1.11 KB, patch)
2015-08-24 11:40 UTC, Stefan Metzmacher
slow: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dron 2015-08-18 07:52:21 UTC
Hello.
FreeBSD 10.1
Samba as AD DC with BIND with DLZ.

Timur updated port to samba 4.2.3 at FreeBSD. After updating samba and other needed components, samba and bind not started with crashes. After some truss and gdb I found, that crash happend when started to work with ldb, tdb files:

gdb output is:
#0  0x00000008048489d8 in rep_strtoll (str=Cannot access memory at address 0x7fffffbfeff8
) at ../lib/replace/replace.c:522
522     ../lib/replace/replace.c: No such file or directory.
        in ../lib/replace/replace.c
[New Thread 802006400 (LWP 100093/python2.7)]
(gdb) where
#0  0x00000008048489d8 in rep_strtoll (str=Cannot access memory at address 0x7fffffbfeff8
) at ../lib/replace/replace.c:522
#1  0x00000008048489f3 in rep_strtoll (str=0x7fffffffa1e4 "16", endptr=0x7fffffffa1d8, base=10) at ../lib/replace/replace.c:523

So, looking through lib/replace/replace.c in last and previous versions of ldb and tdb I found, that function rep_strtoll was changed from

#ifdef HAVE_BSD_STRTOLL
#ifdef HAVE_STRTOQ
long long int rep_strtoll(const char *str, char **endptr, int base)
{
        long long int nb = strtoq(str, endptr, base);
        /* In linux EINVAL is only returned if base is not ok */
        if (errno == EINVAL) {
                if (base == 0 || (base >1 && base <37)) {
                        /* Base was ok so it's because we were not
                         * able to make the convertion.
                         * Let's reset errno.
                         */
                        errno = 0;
                }
        }
        return nb;
}
#else
#error "You need the strtoq function"
#endif /* HAVE_STRTOQ */
#endif /* HAVE_BSD_STRTOLL */

to

#ifdef HAVE_BSD_STRTOLL
long long int rep_strtoll(const char *str, char **endptr, int base)
{
        long long int nb = strtoll(str, endptr, base);
        /* With glibc EINVAL is only returned if base is not ok */
        if (errno == EINVAL) {
                if (base == 0 || (base >1 && base <37)) {
                        /* Base was ok so it's because we were not
                         * able to make the convertion.
                         * Let's reset errno.
                         */
                        errno = 0;
                }
        }
        return nb;
}
#endif /* HAVE_BSD_STRTOLL */

And new code is crashing at FreeBSD. 
Also I checked replace.c in samba 4.2.3 and ntdb, there old code.
Checked ldbedit, classicupgrade procedures, they also crashed.

This needs to be fixed ASAP, because current state of samba and needed components in FreeBSD is fully unusable as AD DC.
Comment 1 Volker Lendecke 2015-08-18 19:14:34 UTC
Created attachment 11350 [details]
Patch

This patch is for master, but it should similarly apply to 4.2
Comment 2 Ralph Böhme 2015-08-19 06:20:01 UTC
(In reply to Volker Lendecke from comment #1)
lgtm & pushed to master.
Comment 3 Dron 2015-08-19 09:03:43 UTC
Edited replace.c according to new code and patch. Final code is:

#ifdef HAVE_BSD_STRTOLL
#undef strtoll
long long int rep_strtoll(const char *str, char **endptr, int base)
{
        long long int nb = strtoll(str, endptr, base);
        /* With glibc EINVAL is only returned if base is not ok */
        if (errno == EINVAL) {
                if (base == 0 || (base >1 && base <37)) {
                        /* Base was ok so it's because we were not
                         * able to make the convertion.
                         * Let's reset errno.
                         */
                        errno = 0;
                }
        }
        return nb;
}
#endif /* HAVE_BSD_STRTOLL */

replace.c was edited in next FreeBSD ports:
net/samba42
databases/ldb
databases/ntdb
databases/tdb
devel/talloc
devel/tevent

Seems these all ports which include lib/replace but files in ports have differences (defferent sizes). Timur needs to review ports with lib/replace and update to last version.

After recompiling, samba-tool dbcheck passed, samba and bind started without problem with data from 4.2.2, ldbedit normally open files. Clean provision and classicupgrade I didn't test yet.

Also seems there is mistype in patch description. Function name is rep_strtoll, not rep_strtoull.

Thanks!
Comment 4 Stefan Metzmacher 2015-08-19 15:08:19 UTC
rep_strtoull() needs the same fix.
Comment 5 Dron 2015-08-20 08:47:33 UTC
Hello.
So, rep_srttoull needs to be changed from that:

#ifdef HAVE_BSD_STRTOLL
#ifdef HAVE_STRTOUQ
unsigned long long int rep_strtoull(const char *str, char **endptr, int base)
{
        unsigned long long int nb = strtouq(str, endptr, base);
        /* In linux EINVAL is only returned if base is not ok */
        if (errno == EINVAL) {
                if (base == 0 || (base >1 && base <37)) {
                        /* Base was ok so it's because we were not
                         * able to make the convertion.
                         * Let's reset errno.
                         */
                        errno = 0;
                }
        }
        return nb;
}
#else
#error "You need the strtouq function"
#endif /* HAVE_STRTOUQ */
#endif /* HAVE_BSD_STRTOLL */
#endif /* HAVE_STRTOULL */

to this

#ifdef HAVE_BSD_STRTOLL
#ifdef HAVE_STRTOUQ
#undef strtoull
unsigned long long int rep_strtoull(const char *str, char **endptr, int base)
{
        unsigned long long int nb = strtouq(str, endptr, base);
        /* In linux EINVAL is only returned if base is not ok */
        if (errno == EINVAL) {
                if (base == 0 || (base >1 && base <37)) {
                        /* Base was ok so it's because we were not
                         * able to make the convertion.
                         * Let's reset errno.
                         */
                        errno = 0;
                }
        }
        return nb;
}
#else
#error "You need the strtouq function"
#endif /* HAVE_STRTOUQ */
#endif /* HAVE_BSD_STRTOLL */
#endif /* HAVE_STRTOULL */


Am I right?
Thanks!
Comment 6 Stefan Metzmacher 2015-08-21 10:54:20 UTC
(In reply to Dron from comment #5)

Looks correct
Comment 7 Volker Lendecke 2015-08-24 05:51:26 UTC
(In reply to Stefan Metzmacher from comment #6)

Really? The endless recursion in rep_strtoll came from the #define strtoll rep_strtoll which thus called itself. In rep_strtoull we call strtouq, thus we don't have that recursion. Whether calling strtouq is the right thing to do here is a different question, but I don't see how the same bug would happen for rep_strtoull.
Comment 8 Stefan Metzmacher 2015-08-24 07:40:57 UTC
(In reply to Volker Lendecke from comment #7)

Ok, I just realized that the following patches are not yet in master:

https://lists.samba.org/archive/samba-technical/2015-July/108587.html
https://lists.samba.org/archive/samba-technical/2015-July/108588.html
https://lists.samba.org/archive/samba-technical/2015-July/108589.html


https://lists.samba.org/archive/samba-technical/2015-July/108587.html
would introduce the same problem for rep_strtoull().

Can you push your fix for rep_strtoll() together with a fixed version of
https://lists.samba.org/archive/samba-technical/2015-July/108587.html
and the two additional changes to master?

All reviewed-by: me

Thanks!
Comment 9 Stefan Metzmacher 2015-08-24 07:44:31 UTC
(In reply to Stefan Metzmacher from comment #8)

Ok, now I realized that you fix is already in master...
But can you look at the rest? Thanks!
Comment 10 Volker Lendecke 2015-08-24 08:24:23 UTC
(In reply to Stefan Metzmacher from comment #9)

No, sorry, I have already deleted the patches and I don't have the energy right now to manually re-craft them from the list archive.
Comment 11 Volker Lendecke 2015-08-24 08:26:42 UTC
Closing this bug as fixed. The fact that some patches might be missing is definitely a different issue than the infinite recursion in rep_strtoll
Comment 12 Stefan Metzmacher 2015-08-24 11:40:27 UTC
Created attachment 11359 [details]
Patch for v4-3-test
Comment 13 Stefan Metzmacher 2015-08-24 11:40:48 UTC
We need this for 4.3
Comment 14 Stefan Metzmacher 2015-08-24 12:13:25 UTC
Pushed to autobuild-v4-3-test
Comment 15 Dron 2015-08-24 13:01:06 UTC
Hello.
What about 4.2, will be patch for this branch? Will it be included in 4.2.4?
Comment 16 Stefan Metzmacher 2015-08-24 13:04:53 UTC
(In reply to Dron from comment #15)

Once we've sorted everthing out for master and v4-3-test, we can backport
all fixes to v4-2-test.
Comment 17 Dron 2015-08-24 13:05:59 UTC
Ok, Thanks!
Comment 18 Stefan Metzmacher 2015-08-31 08:20:24 UTC
Pushed to v4-3-test.

Keep open for v4-2-test
Comment 19 Volker Lendecke 2015-09-06 15:37:46 UTC
*** Bug 11497 has been marked as a duplicate of this bug. ***
Comment 20 Andrew Bartlett 2016-08-01 19:40:02 UTC
Samba 4.2 is now in security fixes only mode.  Closing again.

Fixed in master with 62d08ea715d1664a7600250abbd1dc83f3a33a4c and 4.3 with 02d549ab4da7b9689f22dd719bdd8b9d66f61d60