Skip to content

Commit 426fe2f

Browse files
GirgiasGirgias
committed
Standardize behaviour for int message number between functions
1 parent e45cc31 commit 426fe2f

8 files changed

+300
-93
lines changed

‎NEWS

+2
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@ PHP NEWS
1515

1616
- IMAP
1717
. Fixed bug #80438 (imap_msgno() incorrectly warns and return false on valid UIDs in PHP 8.0.0). (girgias)
18+
. Fix a regression with valid UIDs in imap_savebody() (girgias)
19+
. Make warnings for invalid message numbers/UIDs between functions consistent (girgias)
1820

1921
- Intl:
2022
. Fixed bug #80425 (MessageFormatAdapter::getArgTypeList redefined). (Nikita)

‎ext/imap/php_imap.c

+45-85
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,28 @@ static int le_imap;
152152
RETURN_FALSE; \
153153
} \
154154

155+
#define PHP_IMAP_CHECK_MSGNO_MAYBE_UID_PRE_FLAG_CHECKS(msgindex, arg_pos) \
156+
if (msgindex < 1) { \
157+
zend_argument_value_error(arg_pos, "must be greater than 0"); \
158+
RETURN_THROWS(); \
159+
} \
160+
161+
#define PHP_IMAP_CHECK_MSGNO_MAYBE_UID_POST_FLAG_CHECKS(msgindex, arg_pos, func_flags, uid_flag) \
162+
if (func_flags & uid_flag) { \
163+
/* This should be cached; if it causes an extra RTT to the IMAP server, */ \
164+
/* then that's the price we pay for making sure we don't crash. */ \
165+
unsigned int msg_no_from_uid = mail_msgno(imap_le_struct->imap_stream, msgindex); \
166+
if (msg_no_from_uid == 0) { \
167+
php_error_docref(NULL, E_WARNING, "UID does not exist"); \
168+
RETURN_FALSE; \
169+
} \
170+
} else { \
171+
if (((unsigned) msgindex) > imap_le_struct->imap_stream->nmsgs) { \
172+
php_error_docref(NULL, E_WARNING, "Bad message number"); \
173+
RETURN_FALSE; \
174+
} \
175+
} \
176+
155177
/* {{{ mail_close_it */
156178
static void mail_close_it(zend_resource *rsrc)
157179
{
@@ -1258,7 +1280,6 @@ PHP_FUNCTION(imap_body)
12581280
zval *streamind;
12591281
zend_long msgno, flags = 0;
12601282
pils *imap_le_struct;
1261-
unsigned long msgindex;
12621283
char *body;
12631284
unsigned long body_len = 0;
12641285

@@ -1270,32 +1291,15 @@ PHP_FUNCTION(imap_body)
12701291
RETURN_THROWS();
12711292
}
12721293

1273-
if (msgno < 1) {
1274-
zend_argument_value_error(2, "must be greater than 0");
1275-
RETURN_THROWS();
1276-
}
1294+
PHP_IMAP_CHECK_MSGNO_MAYBE_UID_PRE_FLAG_CHECKS(msgno, 2);
12771295

12781296
if (flags && ((flags & ~(FT_UID|FT_PEEK|FT_INTERNAL)) != 0)) {
12791297
zend_argument_value_error(3, "must be a bitmask of FT_UID, FT_PEEK, and FT_INTERNAL");
12801298
RETURN_THROWS();
12811299
}
12821300

1283-
if (flags && (flags & FT_UID)) {
1284-
/* This should be cached; if it causes an extra RTT to the
1285-
IMAP server, then that's the price we pay for making
1286-
sure we don't crash. */
1287-
msgindex = mail_msgno(imap_le_struct->imap_stream, msgno);
1288-
if (msgindex == 0) {
1289-
php_error_docref(NULL, E_WARNING, "UID does not exist");
1290-
RETURN_FALSE;
1291-
}
1292-
} else {
1293-
msgindex = (unsigned long) msgno;
1294-
}
1295-
1296-
PHP_IMAP_CHECK_MSGNO(msgindex, 2);
1301+
PHP_IMAP_CHECK_MSGNO_MAYBE_UID_POST_FLAG_CHECKS(msgno, 2, flags, FT_UID);
12971302

1298-
/* TODO Shouldn't this pass msgindex??? */
12991303
body = mail_fetchtext_full (imap_le_struct->imap_stream, msgno, &body_len, flags);
13001304
if (body_len == 0) {
13011305
RETVAL_EMPTY_STRING();
@@ -1305,6 +1309,7 @@ PHP_FUNCTION(imap_body)
13051309
}
13061310
/* }}} */
13071311

1312+
/* TODO UID Tests */
13081313
/* {{{ Copy specified message to a mailbox */
13091314
PHP_FUNCTION(imap_mail_copy)
13101315
{
@@ -1334,6 +1339,7 @@ PHP_FUNCTION(imap_mail_copy)
13341339
}
13351340
/* }}} */
13361341

1342+
/* TODO UID Tests */
13371343
/* {{{ Move specified message to a mailbox */
13381344
PHP_FUNCTION(imap_mail_move)
13391345
{
@@ -1605,13 +1611,15 @@ PHP_FUNCTION(imap_delete)
16051611
RETURN_THROWS();
16061612
}
16071613

1614+
// TODO Check sequence validity?
1615+
16081616
if (flags && ((flags & ~FT_UID) != 0)) {
16091617
zend_argument_value_error(3, "must be FT_UID or 0");
16101618
RETURN_THROWS();
16111619
}
16121620

16131621
mail_setflag_full(imap_le_struct->imap_stream, ZSTR_VAL(sequence), "\\DELETED", flags);
1614-
RETVAL_TRUE;
1622+
RETURN_TRUE;
16151623
}
16161624
/* }}} */
16171625

@@ -1882,7 +1890,6 @@ PHP_FUNCTION(imap_fetchstructure)
18821890
zend_long msgno, flags = 0;
18831891
pils *imap_le_struct;
18841892
BODY *body;
1885-
int msgindex;
18861893

18871894
if (zend_parse_parameters(ZEND_NUM_ARGS(), "rl|l", &streamind, &msgno, &flags) == FAILURE) {
18881895
RETURN_THROWS();
@@ -1892,34 +1899,18 @@ PHP_FUNCTION(imap_fetchstructure)
18921899
RETURN_THROWS();
18931900
}
18941901

1895-
if (msgno < 1) {
1896-
zend_argument_value_error(2, "must be greater than 0");
1897-
RETURN_THROWS();
1898-
}
1902+
PHP_IMAP_CHECK_MSGNO_MAYBE_UID_PRE_FLAG_CHECKS(msgno, 2);
18991903

19001904
if (flags && ((flags & ~FT_UID) != 0)) {
19011905
zend_argument_value_error(3, "must be FT_UID or 0");
19021906
RETURN_THROWS();
19031907
}
19041908

1905-
object_init(return_value);
1909+
PHP_IMAP_CHECK_MSGNO_MAYBE_UID_POST_FLAG_CHECKS(msgno, 2, flags, FT_UID);
19061910

1907-
if (flags & FT_UID) {
1908-
/* This should be cached; if it causes an extra RTT to the
1909-
IMAP server, then that's the price we pay for making
1910-
sure we don't crash. */
1911-
msgindex = mail_msgno(imap_le_struct->imap_stream, msgno);
1912-
if (msgindex == 0) {
1913-
php_error_docref(NULL, E_WARNING, "UID does not exist");
1914-
RETURN_FALSE;
1915-
}
1916-
} else {
1917-
msgindex = msgno;
1918-
}
1919-
PHP_IMAP_CHECK_MSGNO(msgindex, 2);
1911+
object_init(return_value);
19201912

1921-
/* TODO Shouldn't this pass msgindex??? */
1922-
mail_fetchstructure_full(imap_le_struct->imap_stream, msgno, &body , (ZEND_NUM_ARGS() == 3 ? flags : NIL));
1913+
mail_fetchstructure_full(imap_le_struct->imap_stream, msgno, &body , flags);
19231914

19241915
if (!body) {
19251916
php_error_docref(NULL, E_WARNING, "No body information available");
@@ -1948,20 +1939,14 @@ PHP_FUNCTION(imap_fetchbody)
19481939
RETURN_THROWS();
19491940
}
19501941

1951-
if (msgno < 1) {
1952-
zend_argument_value_error(2, "must be greater than 0");
1953-
RETURN_THROWS();
1954-
}
1942+
PHP_IMAP_CHECK_MSGNO_MAYBE_UID_PRE_FLAG_CHECKS(msgno, 2);
19551943

19561944
if (flags && ((flags & ~(FT_UID|FT_PEEK|FT_INTERNAL)) != 0)) {
19571945
zend_argument_value_error(4, "must be a bitmask of FT_UID, FT_PEEK, and FT_INTERNAL");
19581946
RETURN_THROWS();
19591947
}
19601948

1961-
if (!(flags & FT_UID)) {
1962-
/* only perform the check if the msgno is a message number and not a UID */
1963-
PHP_IMAP_CHECK_MSGNO(msgno, 2);
1964-
}
1949+
PHP_IMAP_CHECK_MSGNO_MAYBE_UID_POST_FLAG_CHECKS(msgno, 2, flags, FT_UID);
19651950

19661951
body = mail_fetchbody_full(imap_le_struct->imap_stream, msgno, ZSTR_VAL(sec), &len, flags);
19671952

@@ -1989,24 +1974,18 @@ PHP_FUNCTION(imap_fetchmime)
19891974
RETURN_THROWS();
19901975
}
19911976

1992-
if (msgno < 1) {
1993-
zend_argument_value_error(2, "must be greater than 0");
1977+
if ((imap_le_struct = (pils *)zend_fetch_resource(Z_RES_P(streamind), "imap", le_imap)) == NULL) {
19941978
RETURN_THROWS();
19951979
}
19961980

1981+
PHP_IMAP_CHECK_MSGNO_MAYBE_UID_PRE_FLAG_CHECKS(msgno, 2);
1982+
19971983
if (flags && ((flags & ~(FT_UID|FT_PEEK|FT_INTERNAL)) != 0)) {
19981984
zend_argument_value_error(4, "must be a bitmask of FT_UID, FT_PEEK, and FT_INTERNAL");
19991985
RETURN_THROWS();
20001986
}
20011987

2002-
if ((imap_le_struct = (pils *)zend_fetch_resource(Z_RES_P(streamind), "imap", le_imap)) == NULL) {
2003-
RETURN_THROWS();
2004-
}
2005-
2006-
if (!(flags & FT_UID)) {
2007-
/* only perform the check if the msgno is a message number and not a UID */
2008-
PHP_IMAP_CHECK_MSGNO(msgno, 2);
2009-
}
1988+
PHP_IMAP_CHECK_MSGNO_MAYBE_UID_POST_FLAG_CHECKS(msgno, 2, flags, FT_UID);
20101989

20111990
body = mail_fetch_mime(imap_le_struct->imap_stream, msgno, ZSTR_VAL(sec), &len, flags);
20121991

@@ -2037,18 +2016,14 @@ PHP_FUNCTION(imap_savebody)
20372016
RETURN_THROWS();
20382017
}
20392018

2040-
// TODO Fix for UID and normal MSGNO
2041-
//PHP_IMAP_CHECK_MSGNO(msgno, 3);
2019+
PHP_IMAP_CHECK_MSGNO_MAYBE_UID_PRE_FLAG_CHECKS(msgno, 3)
20422020

20432021
if (flags && ((flags & ~(FT_UID|FT_PEEK|FT_INTERNAL)) != 0)) {
20442022
zend_argument_value_error(5, "must be a bitmask of FT_UID, FT_PEEK, and FT_INTERNAL");
20452023
RETURN_THROWS();
20462024
}
20472025

2048-
// TODO Drop this?
2049-
if (!imap_le_struct) {
2050-
RETURN_FALSE;
2051-
}
2026+
PHP_IMAP_CHECK_MSGNO_MAYBE_UID_POST_FLAG_CHECKS(msgno, 3, flags, FT_UID);
20522027

20532028
switch (Z_TYPE_P(out))
20542029
{
@@ -2772,9 +2747,8 @@ PHP_FUNCTION(imap_sort)
27722747
PHP_FUNCTION(imap_fetchheader)
27732748
{
27742749
zval *streamind;
2775-
zend_long msgno, flags = 0L;
2750+
zend_long msgno, flags = 0;
27762751
pils *imap_le_struct;
2777-
int msgindex;
27782752

27792753
if (zend_parse_parameters(ZEND_NUM_ARGS(), "rl|l", &streamind, &msgno, &flags) == FAILURE) {
27802754
RETURN_THROWS();
@@ -2784,30 +2758,16 @@ PHP_FUNCTION(imap_fetchheader)
27842758
RETURN_THROWS();
27852759
}
27862760

2787-
// TODO Check for msgno < 1 now or wait later for PHP_IMAP_CHECK_MSGNO check?
2761+
PHP_IMAP_CHECK_MSGNO_MAYBE_UID_PRE_FLAG_CHECKS(msgno, 2);
27882762

27892763
if (flags && ((flags & ~(FT_UID|FT_INTERNAL|FT_PREFETCHTEXT)) != 0)) {
27902764
zend_argument_value_error(3, "must be a bitmask of FT_UID, FT_PREFETCHTEXT, and FT_INTERNAL");
27912765
RETURN_THROWS();
27922766
}
27932767

2794-
if (flags & FT_UID) {
2795-
/* This should be cached; if it causes an extra RTT to the
2796-
IMAP server, then that's the price we pay for making sure
2797-
we don't crash. */
2798-
msgindex = mail_msgno(imap_le_struct->imap_stream, msgno);
2799-
if (msgindex == 0) {
2800-
php_error_docref(NULL, E_WARNING, "UID does not exist");
2801-
RETURN_FALSE;
2802-
}
2803-
} else {
2804-
msgindex = msgno;
2805-
}
2806-
2807-
PHP_IMAP_CHECK_MSGNO(msgindex, 2);
2768+
PHP_IMAP_CHECK_MSGNO_MAYBE_UID_POST_FLAG_CHECKS(msgno, 2, flags, FT_UID);
28082769

2809-
/* TODO Check shouldn't this pass msgindex???? */
2810-
RETVAL_STRING(mail_fetchheader_full(imap_le_struct->imap_stream, msgno, NIL, NIL, (ZEND_NUM_ARGS() == 3 ? flags : NIL)));
2770+
RETVAL_STRING(mail_fetchheader_full(imap_le_struct->imap_stream, msgno, NIL, NIL, flags));
28112771
}
28122772
/* }}} */
28132773

‎ext/imap/tests/imap_body.phpt renamed to ‎ext/imap/tests/imap_body_errors.phpt

+12-8
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
--TEST--
2-
imap_body() ValueError
2+
imap_body() errors: ValueError and Warnings
33
--CREDITS--
44
Paul Sohier
55
#phptestfest utrecht
@@ -12,28 +12,29 @@ require_once(__DIR__.'/setup/skipif.inc');
1212

1313
require_once(__DIR__.'/setup/imap_include.inc');
1414

15-
$imap_stream = setup_test_mailbox("imapbodyvalueerror", 0);
15+
$imap_mail_box = setup_test_mailbox("imapbodyerror", 0);
1616

1717
try {
18-
imap_body($imap_stream,-1);
18+
imap_body($imap_mail_box, -1);
1919
} catch (\ValueError $e) {
2020
echo $e->getMessage() . \PHP_EOL;
2121
}
2222
try {
23-
imap_body($imap_stream,1,-1);
23+
imap_body($imap_mail_box, 1, -1);
2424
} catch (\ValueError $e) {
2525
echo $e->getMessage() . \PHP_EOL;
2626
}
2727

28-
//Access not existing
29-
var_dump(imap_body($imap_stream, 255, FT_UID));
28+
// Access not existing
29+
var_dump(imap_body($imap_mail_box, 255));
30+
var_dump(imap_body($imap_mail_box, 255, FT_UID));
3031

31-
imap_close($imap_stream);
32+
imap_close($imap_mail_box);
3233

3334
?>
3435
--CLEAN--
3536
<?php
36-
$mailbox_suffix = 'imapbodyvalueerror';
37+
$mailbox_suffix = 'imapbodyerror';
3738
require_once(__DIR__ . '/setup/clean.inc');
3839
?>
3940
--EXPECTF--
@@ -42,5 +43,8 @@ New mailbox created
4243
imap_body(): Argument #2 ($message_num) must be greater than 0
4344
imap_body(): Argument #3 ($flags) must be a bitmask of FT_UID, FT_PEEK, and FT_INTERNAL
4445

46+
Warning: imap_body(): Bad message number in %s on line %d
47+
bool(false)
48+
4549
Warning: imap_body(): UID does not exist in %s on line %d
4650
bool(false)

‎ext/imap/tests/imap_fetchbody_errors.phpt

+49
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
--TEST--
2+
imap_fetchbody() errors: ValueError and Warnings
3+
--SKIPIF--
4+
<?php
5+
require_once(__DIR__.'/setup/skipif.inc');
6+
?>
7+
--FILE--
8+
<?php
9+
10+
require_once(__DIR__.'/setup/imap_include.inc');
11+
12+
$imap_mail_box = setup_test_mailbox("imapfetchbodyerrors", 0);
13+
14+
$section = '';
15+
16+
try {
17+
imap_fetchbody($imap_mail_box, -1, $section);
18+
} catch (\ValueError $e) {
19+
echo $e->getMessage() . \PHP_EOL;
20+
}
21+
try {
22+
imap_fetchbody($imap_mail_box, 1, $section, -1);
23+
} catch (\ValueError $e) {
24+
echo $e->getMessage() . \PHP_EOL;
25+
}
26+
27+
// Access not existing
28+
var_dump(imap_fetchbody($imap_mail_box, 255, $section));
29+
var_dump(imap_fetchbody($imap_mail_box, 255, $section, FT_UID));
30+
31+
imap_close($imap_mail_box);
32+
33+
?>
34+
--CLEAN--
35+
<?php
36+
$mailbox_suffix = 'imapfetchbodyerrors';
37+
require_once(__DIR__ . '/setup/clean.inc');
38+
?>
39+
--EXPECTF--
40+
Create a temporary mailbox and add 0 msgs
41+
New mailbox created
42+
imap_fetchbody(): Argument #2 ($message_num) must be greater than 0
43+
imap_fetchbody(): Argument #4 ($flags) must be a bitmask of FT_UID, FT_PEEK, and FT_INTERNAL
44+
45+
Warning: imap_fetchbody(): Bad message number in %s on line %d
46+
bool(false)
47+
48+
Warning: imap_fetchbody(): UID does not exist in %s on line %d
49+
bool(false)

0 commit comments

Comments
 (0)