Date: Wed, 26 Jun 2013 03:14:44 +0200 (CEST) From: Dan Lukes <dan@obluda.cz> To: FreeBSD-gnats-submit@freebsd.org Subject: ports/179989: [ patch ] net/istgt broken linking, broken cast, broken compilation Message-ID: <201306260114.r5Q1Eiqh098799@m9-64.freebsd.cz> Resent-Message-ID: <201306260120.r5Q1K1FE076483@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
>Number: 179989
>Category: ports
>Synopsis: [ patch ] net/istgt broken linking, broken cast, broken compilation
>Confidential: no
>Severity: serious
>Priority: medium
>Responsible: freebsd-ports-bugs
>State: open
>Quarter:
>Keywords:
>Date-Required:
>Class: sw-bug
>Submitter-Id: current-users
>Arrival-Date: Wed Jun 26 01:20:00 UTC 2013
>Closed-Date:
>Last-Modified:
>Originator: Dan Lukes
>Release: FreeBSD
>Organization:
Obludarium
>Environment:
System: FreeBSD
Ports tree fetched on Jun 25 20:33
head/net/istgt/Makefile 306542 2012-10-28 10:03:03Z
>Description:
A: broken linking (if VirtualBox linked against OpenSSL from ports)
the port links against OpenSSL's libcrypto,
but doesn't honor WITH_OPENSSL_PORT=yes
At the same time it links to virtualbox/VBoxRT.so
which is linked to either base's or port's
OpenSSL's libcrypto
if VBoxRT is linked to port's OpenSSL then
istgt binary become linked to
both BASE's and PORT's libcrypto
It may cause program malfunction because of overlapping
symbols and structures with identical name
B: broken cast
There are so many places with the construct like:
ISTGT_WARNLOG("Connection reset by peer (%s,time=%d)\n",
conn->initiator_name, (int)difftime(now, start));
cast from double to int is invalid
althougth it raise warning only, resulting code may not
work on some architectures at all
C: broken compilation (just for completeness)
it doesn't compile with VBOXVD option turned on at all
firing error:
/usr/ports/emulators/virtualbox-ose/work/VirtualBox-4.2.14/include/VBox/vd.h:182:
error: expected specifier-qualifier-list before 'PARTITIONING_TYPE'
>How-To-Repeat:
A: compile VirtualBox with WITH_OPENSSL_PORT=yes, then compile istgt
see warning past linking command:
cc -Wl,-rpath,/usr/local/lib/virtualbox -o istgtcontrol istgtcontrol.o istgt_conf.o istgt_log.o istgt_sock.o istgt_misc.o istgt_md5.o -lcam -lcrypto -lpthread /usr/local/lib/virtualbox/VBoxDDU.so /usr/local/lib/virtualbox/VBoxRT.so
B: see several warnings during compilation:
cast from function call of type 'double' to non-matching type 'int'
C: turn VBOXVD option on. Compilation will abort with error mentioned
in "Description"
>Fix:
A: 1. the libcrypto is used just for the purpose of calculation of MD5 digest.
istgt needs to be linked against same version (base/ports) of libcrypto
as virtualbox/VBoxRT.so has been linked to
2. dependency to port's OpenSSL should be recorded
if istgt become linked to it
Even if we modify the port to link against appropriate
libcrypto and record dependency if necesarry, then
administrator need to identify what version of libcrypto
should be used or a complex autodetection logic needs
to be implemented
So I suggest different approach to avoid conflict.
I replaced libcrypto's MD5_*() by libmd's MD5*().
No conflict may occur, no dependency needs to be recorded,
no administrator decision nor autodetection logic necesarry
at all.
Required changes are small:
a) MD5_Init/MD5_Update/MD5_Final functions
are replaced by
MD5Init/MD5Update/MD5Final
b) openssl/md5.h header reference replaced by reference
to sys/types.h and md5.h
c) references to libcrypto replaced by reference to libmd.
See patch-DAN-replacecrypto
B: printf double as true double not int
using %.0f format
As construct in question are used for error logging only,
not in casual code paths, such change should not affect
the performance nor CPU utilisation
See patch-DAN-invalidcast
C: this bug is not in istgt code but bug in virtualbox header.
But it doesn't affect compilation of VirtualBox itself.
I include it just for completeness as istgt is only affected
software known to me.
Error is caused by PARTITIONING_TYPE symbol declared as enum in
header file but referenced without enum keyword, e.g.
like typedef's type. The symbol is not referenced as 'enum'
in any place of istgt. Even VirtualBox code (not used during
standard compilation of VirtualBox) references symbol
without 'enum' keyword.
I added typedef wrapper around it's declaration. It's conform to
style of declaration of other enums and structures. I assume
the wrapper has been forgotten here.
See emulators/virtualbox-ose/files/patch-DAN-include-VBox-vd.h
patch
--- patch-DAN-replacecrypto begins here ---
--- src/config.h.in.orig 2012-08-19 06:51:15.000000000 +0200
+++ src/config.h.in 2013-06-26 01:30:15.000000000 +0200
@@ -54,8 +54,8 @@
/* Define to 1 if you have the `cam' library (-lcam). */
#undef HAVE_LIBCAM
-/* Define to 1 if you have the `crypto' library (-lcrypto). */
-#undef HAVE_LIBCRYPTO
+/* Define to 1 if you have the `md' library (-lmd). */
+#undef HAVE_LIBMD
/* Define to 1 if you have the `pthread' library (-lpthread). */
#undef HAVE_LIBPTHREAD
--- src/istgt_md5.c.orig 2010-01-02 18:57:26.000000000 +0100
+++ src/istgt_md5.c 2013-06-26 01:35:24.000000000 +0200
@@ -33,7 +33,8 @@
#include <stdint.h>
#include <stddef.h>
-#include <openssl/md5.h>
+#include <sys/types.h>
+#include <md5.h>
#include "istgt.h"
#include "istgt_md5.h"
@@ -41,34 +42,28 @@
int
istgt_md5init(ISTGT_MD5CTX *md5ctx)
{
- int rc;
-
if (md5ctx == NULL)
return -1;
- rc = MD5_Init(&md5ctx->md5ctx);
- return rc;
+ MD5Init(&md5ctx->md5ctx);
+ return 1;
}
int
istgt_md5final(void *md5, ISTGT_MD5CTX *md5ctx)
{
- int rc;
-
if (md5ctx == NULL || md5 == NULL)
return -1;
- rc = MD5_Final(md5, &md5ctx->md5ctx);
- return rc;
+ MD5Final(md5, &md5ctx->md5ctx);
+ return 1;
}
int
istgt_md5update(ISTGT_MD5CTX *md5ctx, const void *data, size_t len)
{
- int rc;
-
if (md5ctx == NULL)
return -1;
if (data == NULL || len <= 0)
return 0;
- rc = MD5_Update(&md5ctx->md5ctx, data, len);
- return rc;
+ MD5Update(&md5ctx->md5ctx, data, len);
+ return 1;
}
--- src/istgt_md5.h.orig 2010-01-02 18:57:26.000000000 +0100
+++ src/istgt_md5.h 2013-06-26 01:20:46.000000000 +0200
@@ -30,7 +30,8 @@
#include <stddef.h>
-#include <openssl/md5.h>
+#include <sys/types.h>
+#include <md5.h>
#define ISTGT_MD5DIGEST_LEN MD5_DIGEST_LENGTH
--- configure.orig 2012-08-24 12:19:24.000000000 +0200
+++ configure 2013-06-26 01:23:49.000000000 +0200
@@ -3472,13 +3472,13 @@
fi
-{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for MD5_Update in -lcrypto" >&5
-$as_echo_n "checking for MD5_Update in -lcrypto... " >&6; }
-if ${ac_cv_lib_crypto_MD5_Update+:} false; then :
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for MD5Update in -lmd" >&5
+$as_echo_n "checking for MD5Update in -lmd... " >&6; }
+if ${ac_cv_lib_crypto_MD5Update+:} false; then :
$as_echo_n "(cached) " >&6
else
ac_check_lib_save_LIBS=$LIBS
-LIBS="-lcrypto $LIBS"
+LIBS="-lmd $LIBS"
cat confdefs.h - <<_ACEOF >conftest.$ac_ext
/* end confdefs.h. */
@@ -3488,32 +3488,32 @@
#ifdef __cplusplus
extern "C"
#endif
-char MD5_Update ();
+char MD5Update ();
int
main ()
{
-return MD5_Update ();
+return MD5Update ();
;
return 0;
}
_ACEOF
if ac_fn_c_try_link "$LINENO"; then :
- ac_cv_lib_crypto_MD5_Update=yes
+ ac_cv_lib_crypto_MD5Update=yes
else
- ac_cv_lib_crypto_MD5_Update=no
+ ac_cv_lib_crypto_MD5Update=no
fi
rm -f core conftest.err conftest.$ac_objext \
conftest$ac_exeext conftest.$ac_ext
LIBS=$ac_check_lib_save_LIBS
fi
-{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_lib_crypto_MD5_Update" >&5
-$as_echo "$ac_cv_lib_crypto_MD5_Update" >&6; }
-if test "x$ac_cv_lib_crypto_MD5_Update" = xyes; then :
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_lib_crypto_MD5Update" >&5
+$as_echo "$ac_cv_lib_crypto_MD5Update" >&6; }
+if test "x$ac_cv_lib_crypto_MD5Update" = xyes; then :
cat >>confdefs.h <<_ACEOF
-#define HAVE_LIBCRYPTO 1
+#define HAVE_LIBMD 1
_ACEOF
- LIBS="-lcrypto $LIBS"
+ LIBS="-lmd $LIBS"
fi
--- patch-DAN-replacecrypto ends here ---
--- patch-DAN-invalidcast begins here ---
--- src/istgt_iscsi.c.orig 2012-10-28 00:26:36.000000000 +0200
+++ src/istgt_iscsi.c 2013-06-26 00:44:07.000000000 +0200
@@ -670,16 +670,16 @@
if (rc < 0) {
now = time(NULL);
if (errno == ECONNRESET) {
- ISTGT_WARNLOG("Connection reset by peer (%s,time=%d)\n",
- conn->initiator_name, (int)difftime(now, start));
+ ISTGT_WARNLOG("Connection reset by peer (%s,time=%.0f)\n",
+ conn->initiator_name, difftime(now, start));
conn->state = CONN_STATE_EXITING;
} else if (errno == ETIMEDOUT) {
- ISTGT_WARNLOG("Operation timed out (%s,time=%d)\n",
- conn->initiator_name, (int)difftime(now, start));
+ ISTGT_WARNLOG("Operation timed out (%s,time=%.0f)\n",
+ conn->initiator_name, difftime(now, start));
conn->state = CONN_STATE_EXITING;
} else {
- ISTGT_ERRLOG("iscsi_read() failed (errno=%d,%s,time=%d)\n",
- errno, conn->initiator_name, (int)difftime(now, start));
+ ISTGT_ERRLOG("iscsi_read() failed (errno=%d,%s,time=%.0f)\n",
+ errno, conn->initiator_name, difftime(now, start));
}
return -1;
}
@@ -762,8 +762,8 @@
rc = readv(conn->sock, &iovec[0], 4);
if (rc < 0) {
now = time(NULL);
- ISTGT_ERRLOG("readv() failed (%d,errno=%d,%s,time=%d)\n",
- rc, errno, conn->initiator_name, (int)difftime(now, start));
+ ISTGT_ERRLOG("readv() failed (%d,errno=%d,%s,time=%.0f)\n",
+ rc, errno, conn->initiator_name, difftime(now, start));
return -1;
}
if (rc == 0) {
@@ -1257,8 +1257,8 @@
rc = writev(conn->sock, &iovec[0], 5);
if (rc < 0) {
now = time(NULL);
- ISTGT_ERRLOG("writev() failed (errno=%d,%s,time=%d)\n",
- errno, conn->initiator_name, (int)difftime(now, start));
+ ISTGT_ERRLOG("writev() failed (errno=%d,%s,time=%.0f)\n",
+ errno, conn->initiator_name, difftime(now, start));
return -1;
}
nbytes -= rc;
@@ -3590,9 +3590,9 @@
if (rc < 0) {
now = time(NULL);
ISTGT_ERRLOG("MCS: CmdSN(%u) error ExpCmdSN=%u "
- "(time=%d)\n",
+ "(time=%.0f)\n",
CmdSN, conn->sess->ExpCmdSN,
- (int)difftime(now, start));
+ difftime(now, start));
SESS_MTX_UNLOCK(conn);
return -1;
}
--- src/istgt_lu_disk.c.orig 2012-10-28 00:26:36.000000000 +0200
+++ src/istgt_lu_disk.c 2013-06-26 00:44:05.000000000 +0200
@@ -5288,9 +5288,9 @@
MTX_UNLOCK(&lu_task->trans_mutex);
now = time(NULL);
ISTGT_ERRLOG("timeout trans_cond CmdSN=%u "
- "(time=%d)\n",
+ "(time=%.0f)\n",
lu_task->lu_cmd.CmdSN,
- (int)difftime(now, start));
+ difftime(now, start));
/* timeout */
return -1;
}
@@ -5326,8 +5326,8 @@
if (rc == ETIMEDOUT) {
lu_task->error = 1;
now = time(NULL);
- ISTGT_ERRLOG("timeout trans_cond CmdSN=%u (time=%d)\n",
- lu_task->lu_cmd.CmdSN, (int)difftime(now, start));
+ ISTGT_ERRLOG("timeout trans_cond CmdSN=%u (time=%.0f)\n",
+ lu_task->lu_cmd.CmdSN, difftime(now, start));
return -1;
}
lu_task->error = 1;
--- patch-DAN-invalidcast ends here ---
--- emulators/virtualbox-ose/files/patch-DAN-include-VBox-vd.h begins here ---
--- include/VBox/vd.h~ 2013-06-21 14:24:07.000000000 +0200
+++ include/VBox/vd.h 2013-06-26 00:59:41.000000000 +0200
@@ -154,11 +154,11 @@
* Auxiliary data structure for difference between GPT and MBR
* disks.
*/
-enum PARTITIONING_TYPE
+typedef enum PARTITIONING_TYPE
{
MBR,
GPT
-};
+} PARTITIONING_TYPE;
/**
* Auxiliary data structure for creating raw disks.
--- emulators/virtualbox-ose/files/patch-DAN-include-VBox-vd.h ends here ---
>Release-Note:
>Audit-Trail:
>Unformatted:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201306260114.r5Q1Eiqh098799>